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

add modelextent plot #357

Merged
merged 11 commits into from
Jul 3, 2024
Merged

add modelextent plot #357

merged 11 commits into from
Jul 3, 2024

Conversation

dbrakenhoff
Copy link
Collaborator

@dbrakenhoff dbrakenhoff commented Jun 27, 2024

This PR is originally made by @dbrakenhoff to add a modelextent-plot.

It has been misused by me (@rubencalje) to replace some of the methods from the resample-module to the grid-module. For example the nlmod.resample.get_extent(ds) now is nlmod.grid.get_extent(ds), which I think is more logical. The original methods still function, and the user will see a warning that the old methods are deprecated.

@dbrakenhoff dbrakenhoff self-assigned this Jun 27, 2024
@dbrakenhoff dbrakenhoff added the enhancement New feature or request label Jun 27, 2024
@dbrakenhoff
Copy link
Collaborator Author

Test fails unrelated.

Copy link
Collaborator

@OnnoEbbens OnnoEbbens left a comment

Choose a reason for hiding this comment

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

Nice addition. There are already two functions in nlmod.util to create a polygon from the extent polygon_from_extent and to create a geodataframe from an extent gdf_from_extent. I think these functions are nice to keep and the plot function can call them. Only thing is that we should move them somewhere else because 'util' is not the best place. Probably the resample module is a good place because get_extent is also in there.

nlmod/plot/plot.py Outdated Show resolved Hide resolved
nlmod/plot/plot.py Outdated Show resolved Hide resolved
@rubencalje
Copy link
Collaborator

rubencalje commented Jul 2, 2024

Nice addition. There are already two functions in nlmod.util to create a polygon from the extent polygon_from_extent and to create a geodataframe from an extent gdf_from_extent. I think these functions are nice to keep and the plot function can call them. Only thing is that we should move them somewhere else because 'util' is not the best place. Probably the resample module is a good place because get_extent is also in there.

It is even worse... We also have nlmod.resample.get_extent_polygon. This method also takes a rotated grid into account. Maybe we can merge some of these methods?

@rubencalje
Copy link
Collaborator

Nice addition. There are already two functions in nlmod.util to create a polygon from the extent polygon_from_extent and to create a geodataframe from an extent gdf_from_extent. I think these functions are nice to keep and the plot function can call them. Only thing is that we should move them somewhere else because 'util' is not the best place. Probably the resample module is a good place because get_extent is also in there.

It is even worse... We also have nlmod.resample.get_extent_polygon. This method also takes a rotated grid into account. Maybe we can merge some of these methods?

I moved some of the methods from the resample-module to the grid-module (also see the starting-message of this PR). I also moved extent_to_polygon from the resample-module to the util-module, where it replaces polygon_from_extent, and renamed gdf_from_extent to extent_to_gdf in the util-module. These methods in the util-module can also be used without a grid, and therefore the util-module is a good place for these methods.

All the old methods continue to work for now, and the user will get to see a deprecation-message.

Copy link
Collaborator

@OnnoEbbens OnnoEbbens left a comment

Choose a reason for hiding this comment

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

Compliments for all the proper deprecations. I just have some minor comments.

nlmod/dims/resample.py Show resolved Hide resolved
nlmod/plot/plot.py Show resolved Hide resolved
@dbrakenhoff
Copy link
Collaborator Author

Test fails are unrelated to the last commit. Maybe something to do with the new KNMI stations containing old data? One of the examples is looking for precipitation data from 1893...

Merging this PR, if we need some more fixes, we'll figure that out for the new release PR I'll open shortly.

@dbrakenhoff dbrakenhoff merged commit ca6af64 into dev Jul 3, 2024
1 of 3 checks passed
@dbrakenhoff dbrakenhoff deleted the add_modelextent_plot branch July 3, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants