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

Quadtree read/write, no build #226

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

Quadtree read/write, no build #226

wants to merge 25 commits into from

Conversation

roeldegoede
Copy link
Collaborator

Issue addressed

Fixes #

Explanation

Explain how you addressed the bug/feature request, what choices you made and why.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@roeldegoede
Copy link
Collaborator Author

Done

  • Contains reading/writing of Quadtree files
  • Reading/writing of Quadtree subgrid files
  • New property bbox that can be used to clip for example forcing data
  • Up to date with other open PRs and main branch

Not working/testes:

  • Plot basemap
  • Read results

@roeldegoede roeldegoede requested a review from Leynse November 29, 2024 16:48
@@ -2851,6 +2902,17 @@ def read_grid(self, data_vars: Union[List, str] = None) -> None:
ds.raster.set_crs(epsg)
self.set_grid(ds)

# TODO - fix this properly; but to create overlays in GUIs,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only used by FloodAdapt (and/or DelftDashboard) for the grid-snapping (and eventually visualization of the grid).
@LuukBlom should we make this optional maybe, that would require a change on the FA side.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In FA we access the grid of SfincsModels only via the SfincsModel.quadtree attribute, but we can change that easily to some other function / property, so changes for FloodAdapt should be easy.
We also cache the result of this grid attr/function on our side, so that reduces worries about performance from us.

If you want to put this in a function somewhere to make it optional, but still allow us to retrieve the grid, Im all for it! Please do let me know which function:)

Copy link
Collaborator

@Leynse Leynse left a comment

Choose a reason for hiding this comment

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

Nice work Roel!

Looks good and as discussed

Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed

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