Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort DataFrames inplace. #597

Merged
merged 6 commits into from
Sep 14, 2023
Merged

Sort DataFrames inplace. #597

merged 6 commits into from
Sep 14, 2023

Conversation

evetion
Copy link
Member

@evetion evetion commented Sep 14, 2023

Fixes the performance of saving a Ribasim model in Python.

When we sorted, we assigned the returned dataframe to our model, triggering validation.

Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruff claims:

Further, in many cases, inplace=True does not provide a performance benefit, as pandas will often copy DataFrames in the background.

Would be good to benchmark if that actually improve things.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@visr
Copy link
Member

visr commented Sep 14, 2023

Can confirm that this no longer triggers validation on write.

@visr
Copy link
Member

visr commented Sep 14, 2023

Turns out the Ubuntu CI failure was due to a missing custom sort method. Brought them all up to date with our docs in dc0c288. @evetion already mentioned that the sort algorithm on Linux could be slightly different, and we just luckily ended up with a sorted GeoPackage on Windows.

On top of that, we sort data when reading it into the core as well, so not sorting in Python should not lead to test failures. But those were similarly outdated, fixed in fe64269.

@visr visr merged commit 844cf85 into main Sep 14, 2023
14 of 18 checks passed
@visr visr deleted the fix/sort-inplace branch September 14, 2023 16:45
@evetion evetion mentioned this pull request Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants