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

Buisdrainage #276

Merged
merged 13 commits into from
Oct 23, 2023
Merged

Buisdrainage #276

merged 13 commits into from
Oct 23, 2023

Conversation

rubencalje
Copy link
Collaborator

This pull request adds support for buisdrainage from the NHI-website to nlmod. Also, it fixes some small bugs in code and documentation, for example issue #261

@@ -1024,8 +1024,10 @@ def add_season_timeseries(
package,
summer_months=(4, 5, 6, 7, 8, 9),
filename="season.ts",
seasons=("winter", "summer"),
seasons=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why set seasons to None? The keyword summer_months sort of suggests winter/summer being the only possible two seasons?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed this as a tuple gave a bug, seasons (time_series_namerecord) somehow needed to be a list, and it is not allowed to to add a list as an optional arggument. Therefore when seasons is None, I set it to seasons = ["winter", "summer"]. Maybe better to add two arguments, called winter_name and summer_name?


# make sure crs is set of ds
if ds.rio.crs is None:
ds = ds.rio.write_crs(28992)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use the EPSG definition in nlmod for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Somehow this does not give the same problems as with maps (I believe, but did not thoroughly check). It might break stuff, if we change this. If other data sources are also in 28992, they do not need to be transformed, but if we change the crs of the model Dataset to our own definition, an unwanted transformation is applied. The buisdrainage data is in WGS84, so you may be right. Let's check this in the future.

dbrakenhoff and others added 2 commits October 23, 2023 14:32
- added one docstring, fixed some typos, sort imports
@dbrakenhoff
Copy link
Collaborator

Modifying the EPSG definition doesn't seem to affect the result, but the result does seem somewhat shifted relative to the backgroundmap, though it's difficult to be certain. That being said, the result is the same as on the NHI dataportal website, but it might be shifted there too...

image

@rubencalje rubencalje merged commit f84a399 into dev Oct 23, 2023
2 of 3 checks passed
@rubencalje rubencalje deleted the buisdrainage branch October 23, 2023 20:10
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.

2 participants