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 setup_observation_lines including example and test #114

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

roeldegoede
Copy link
Collaborator

Added functionality to read, write and create crs (cross-section) files to the SfincsModel. Through these cross-sections, discharges are monitored.

In SFINCS, the LineStrings are snapped to all u- and v points for which all the discharged are added together. Discharges per cross-section are added to the sfincs_his.nc.

@roeldegoede roeldegoede added the Enhancement New feature or request label Jul 28, 2023
@roeldegoede roeldegoede linked an issue Jul 28, 2023 that may be closed by this pull request
Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

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

Very nice PR @roeldegoede! Just added to points for discussion, but the PR can be merged as is I think.

Comment on lines +1341 to +1342
# FIXME ensure the catalog is loaded before adding any new entries
self.data_catalog.sources
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, copied this from the setup_observation_points. Can this be related to the fact that self.data_catalog normally keeps tracks of all the used data_sources (something which seems to be disappeared along the way?) and therefore we needed to ensure that the data_catalog was loaded?

Maybe this still originates from the first codes and the problem has been fixed in the meanwhile?

).to_crs(self.crs)

# make sure MultiLineString are converted to LineString
gdf_obs = gdf_obs.explode().reset_index(drop=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

The original data indices are ignored here. Is it important that the indices of the original data are kept? I would think that it might be useful to match indices in the results to your input data. I noticed that also in obs the indices get 'lost' when reading or merging.

Not necessarily something we need to solve here and now, but perhaps good to discuss in conjunction with #115

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair point, since we give the observation points and the observation_lines a name, most of the times that would be sufficient. But maybe keeping the original indices would be nice indeed!

@@ -19,6 +19,7 @@
"bnd": dict(marker="^", markersize=75, c="w", edgecolor="k", annotate=True),
"src": dict(marker=">", markersize=75, c="w", edgecolor="k", annotate=True),
"obs": dict(marker="d", markersize=75, c="w", edgecolor="r", annotate=True),
"crs": dict(linestyle=":", linewidth=1.0, color="k", annotate=False),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit difficult to see with the dep file in the background. Perhaps use red color?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, do you want to stick to the ":" linestyle or are there also better options (exceptthe solid line I guess)? I often notice that dashed or dotted lines do not cover the entire line from start to end, because that may coincide with the "white spaces" in between the icons.

@roeldegoede roeldegoede merged commit d36d8fe into main Aug 3, 2023
3 checks passed
@roeldegoede roeldegoede deleted the enh-setup_observation_lines branch August 3, 2023 07:46
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.

ENH: add setup_cross_sections
2 participants