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

Regular subgrid #160

Merged
merged 35 commits into from
Apr 10, 2024
Merged

Regular subgrid #160

merged 35 commits into from
Apr 10, 2024

Conversation

Leynse
Copy link
Collaborator

@Leynse Leynse commented Dec 15, 2023

Issue addressed

Fixes #159

Explanation

Copy paste code from quadtree_subgrid, without the quadtree part.
Bugfix leftover issues

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

The new NETCDF subgrid for regular grids still needs some attention. I think that this branch https://github.com/Deltares/SFINCS/tree/netcdf_subgrid_reggrid is a step into the right direction, but we have to check whether those changes are needed.

@roeldegoede
Copy link
Collaborator

@DirkEilander Can you have a look at the changes?

We updated the subgrid formulations, changed the format from binary to netcdf and changed the keyword into nlevels instead of nbins.

I'll add this to the changelog and update tests accordingly afterwards as well

@roeldegoede roeldegoede marked this pull request as ready for review March 20, 2024 15:35
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.

Generally the PR looks good to me! I've left a few small comments. I also have not yet tried actually building a new style subgrid table with the code. Will do so asap.

docs/changelog.rst Outdated Show resolved Hide resolved
hydromt_sfincs/sfincs.py Show resolved Hide resolved
hydromt_sfincs/subgrid.py Outdated Show resolved Hide resolved
hydromt_sfincs/utils.py Show resolved Hide resolved
hydromt_sfincs/subgrid.py Outdated Show resolved Hide resolved
Comment on lines +152 to +153
# TODO: check with Maarten whether this is meant to be different
# difference comes from different discretization of volume bins
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DirkEilander this is still something I would like to discuss with Maarten

@roeldegoede roeldegoede merged commit 75cd92d into main Apr 10, 2024
6 checks passed
@roeldegoede roeldegoede deleted the regular_subgrid branch April 10, 2024 15:48
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.

upgrade subgrid in SFINCS to v08
5 participants