-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update package #30
Update package #30
Conversation
from .. import util, cache | ||
|
||
@cache.cache_netcdf | ||
def get_chd_at_model_edge(model_ds, idomain): |
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.
we should also add a method that does this based on a shape(file)?
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.
Yes, that would not be too hard. The function gdf_to_bool_data_array
from the mgrid
module can probably help with that.
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.
Can be done in as part of a new issue.
nlmod/util.py
Outdated
|
||
1. Read cached dataset and merge this with the current model_ds if all | ||
of the following conditions are satisfied: | ||
return model_ds[list(model_ds.coords)] |
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.
is this a copy, or is it still linked to the original data?
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.
Good question. I am not 100% sure but I think it is a copy. If I later add data variables to model_ds these are not added to the returned dataset from get_model_ds_empty
. I also tried modifying the original coördinates of model_ds but could not do this. Therefore I think it is fine to get an empty model dataset using this code. To be sure we can also add .copy(deep=True)
at the end. This is slower but not extremely slow, see timing below:
What do you think?
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.
Perhaps we can check by checking the memory id for the object, the dataarray and underlying array? id(model_ds["x"])
etc?
But I'm also fine with the .copy()
for now just to be sure.
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.
The id's are the same but they differ everytime I call the id
function which is a bit confusing. I will use copy()
for now
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.
Looks good! Lots of nice changes I think! I think after tests pass it can be merged, but leaving some things i came across anyway:
Comments on notebooks:
- 01 contains reference to broken KNMI api, but this is fixed now i believe.
- 07 set extents equal in resampling comparison plots. (+typo in Rectengular)
- 07 some headings could use a bit more explanation, i.e. how it deals with ties (i.e. when 4 cells are equally close to new cell midpoint)
Also we should mention somewhere this is a breaking update for existing scripts relying on nlmod! |
Yes we should, I can mention it when I draft a new release |
Major update including: