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

Addition of raster functionality #446

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Addition of raster functionality #446

wants to merge 19 commits into from

Conversation

kbonney
Copy link
Collaborator

@kbonney kbonney commented Sep 25, 2024

Summary

This PR adds the function sample_raster to the wntr.gis module. The function samples a raster file at the coordinate points of the provided geodataframe (e.g., junctions from WaterNetworkGIS). This PR only extends the existing API and does not modify it (ie, the inclusion of the PR merits a minor version change at most).

Tests and documentation

One test is added to verify that the raster is correctly sampled. A docstring is included in the function definition. Currently no additional documentation is included, but there could be a subsection added to the Geospatial capabilities user manual page to discuss and exemplify usage of the new function.

Acknowledgement

By contributing to this software project, I acknowledge that I have reviewed the software quality assurance guidelines and that my contributions are submitted under the Revised BSD License.

@kbonney kbonney changed the title Raster Addition of raster functionality Sep 25, 2024
@kbonney
Copy link
Collaborator Author

kbonney commented Sep 25, 2024

@dbhart, can you review this?

Copy link
Collaborator

@dbhart dbhart left a comment

Choose a reason for hiding this comment

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

@kbonney Looks good to me!

@kbonney
Copy link
Collaborator Author

kbonney commented Sep 25, 2024

Discussion points:

  • Do we want users to pass in the raster object or the filepath? I opted to have the user pass in the filepath, so that the function can take care of opening and closing the file. If we have users pass in the raster directly, they may forget to close the connection to the file. Other geospatial capabilities directly ingest the geospatial data, rather than filepaths, but geodataframes are different because they exist in memory whereas the raster object is a connection to the file in storage.
  • Do we want to include any raster plotting functionality? For example, the function might take an existing ax with something like a water network plotted on it, extract the extent, fetch the corresponding raster data within the extent, and plot it using imshow. Something like this might be nice for the saltwater demo.
  • User manual docs?

@dbhart
Copy link
Collaborator

dbhart commented Sep 25, 2024

Note to all - failed tests due to actions runner issue that still needs to be merged into main

@kbonney
Copy link
Collaborator Author

kbonney commented Sep 25, 2024

Ok, thanks @dbhart. Any thoughts on the first discussion point above?

@dbhart
Copy link
Collaborator

dbhart commented Sep 25, 2024

i think that we pass the filename. if the file stays open rather than loading the raster into memory then i agree that we want control over opening and close the file

@coveralls
Copy link

coveralls commented Sep 27, 2024

Coverage Status

coverage: 84.241% (+0.01%) from 84.227%
when pulling 5916b9b on kbonney:raster
into 576dc01 on USEPA:main.

@kbonney
Copy link
Collaborator Author

kbonney commented Oct 4, 2024

@kaklise,
Remade test to have verifiable sample values and to use Net1, also added the test as an example to the user docs.

Copy link
Collaborator

@kaklise kaklise left a comment

Choose a reason for hiding this comment

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

Looks good, just requesting minor updates to documentation

documentation/gis.rst Outdated Show resolved Hide resolved
documentation/gis.rst Show resolved Hide resolved
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.

4 participants