-
Notifications
You must be signed in to change notification settings - Fork 11
Issue #1821: Improved performance for allocation & sparse/empty layer handling. #1912
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
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
d8fc795
Improved performance for allocation & sparse/empty layer handling.
LuukBlom abec816
add changelog entry
LuukBlom 34cf8df
remove unused `spatial_dims` arg
LuukBlom c39005a
set default for drop_empty_layers to True and update tests. Fixed a b…
LuukBlom abaecc6
lint
LuukBlom 761fb4a
revert lineendings change.
LuukBlom 714ce81
fix `_used_layers` to also return None on empty masks
LuukBlom 4ee8e7e
sync docstrings for drop_empty_layers
LuukBlom f97cd84
update edge case test where all layers are empty.
LuukBlom a2f6fc6
implement review comments
LuukBlom 3330bb7
Merge branch 'master' into feat/drop-unused-layers
LuukBlom 98d6ddf
lint
LuukBlom 1dd4332
update dvc config
LuukBlom 317adbc
add enum for LAYERS_USED. Created issue #1923 for letting callers han…
LuukBlom ddd880a
lint
LuukBlom 9b187f2
Swap order for if-elif tree to not error when comparing a GridDataArr…
LuukBlom d082d03
lint
LuukBlom 3361e12
Fix mypy errors and add some extra logger messages
JoerivanEngelen a931f71
Introduce match case system for layers_used_option
JoerivanEngelen e613902
Remove unnecessary sentence at the end of docstring explanation drop_…
JoerivanEngelen 7710b6b
Improve performance for cases where the mask has a lot of timesteps
JoerivanEngelen 26f75d2
Format
JoerivanEngelen 1e0d14b
Update return type docstring
JoerivanEngelen 1908917
Merge branch 'master' into feat/drop-unused-layers
JoerivanEngelen 0626ae7
Do not add scratch file to PR
JoerivanEngelen 5966aec
erge branch 'feat/drop-unused-layers' of github.com:Deltares/imod-pyt…
JoerivanEngelen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| [core] | ||
| remote = minio | ||
| ['remote "minio"'] | ||
| url = s3://imod-python-test-data | ||
| endpointurl = https://s3.deltares.nl | ||
| [core] | ||
| remote = minio | ||
| ['remote "minio"'] | ||
| url = s3://imod-python-test-data | ||
| endpointurl = https://s3.deltares.nl | ||
| allow_anonymous_login = true |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't expect this to happen, which tests were failing because of this? Just wondering what the cause of this is as it might mean we overlooked something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While developing I got errors, but I cannot remember exactly which ones to be honest.
I have run the tests again with this fix commented out, and got 2 errors:
test_prepare\test_cleanup.py::test_cleanup_riv__trimmed_layers[structured & unstructured]This is however the test I added to produce this bug and show it does not error anymore. (Which is good news: all callers pass matching layers to this function.)
The bug (introduced by trimming layers):
bottom has the full model layers, while top/to_align can be a trimmed package with a subset of layers.
Calling
xr.where(~to_align, bottom)will then raise an alignment error since xarray uses join="exact" by default.This is why other places in the code use
xr.align(obj1, obj2, join="left"), which fills missing layers with NaN (mask.py and schemata.py).Since the inputs to xr.where need to be the same shape, and we are interested in the top layers, we can throw away the irrelevant layers from bottom before doing this.