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_lulcmaps_from_vector #320

Merged
merged 5 commits into from
Jan 29, 2025
Merged

add setup_lulcmaps_from_vector #320

merged 5 commits into from
Jan 29, 2025

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Jan 20, 2025

Issue addressed

No issue was created.

Explanation

Often local landuse data is in vector/shapefile format rather than vector. This function allows to use vector by first rasterizing to a high resolution grid (resolution from user) before calling the classic setup_lulcmaps function.

An equivalent setup_lulcmaps_from_vector_with_paddy was not added as users will be able to save the rasterized version of the landuse in the root/maps folder, so they can first call setup_lulcmaps_from vector and then setup_lulcmaps_with_paddy using the previously saved rasterized version of the landuse.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@hboisgon hboisgon requested a review from shartgring January 20, 2025 08:11
Copy link
Collaborator

@shartgring shartgring left a comment

Choose a reason for hiding this comment

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

Hi Hélène! Nice feature, I will test it myself this week to make some nice LULC maps, which I luckily still have to do for a project. These are my comments based on reviewing only the code and not running it yet

@hboisgon hboisgon requested a review from shartgring January 21, 2025 02:13
@hboisgon
Copy link
Contributor Author

hboisgon commented Jan 21, 2025

Thanks for the useful review and comments @shartgring ! I think I adressed it all :) could you review again?
I will adress the failing tests in a separate PR as thay are not due to this branch.

@hboisgon hboisgon mentioned this pull request Jan 23, 2025
4 tasks
Copy link
Collaborator

@shartgring shartgring 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 to me! Only 1 thing is missing: I noticed that the LULC map (wflow_landuse) is not clipped to the basin, but to the bounding box. The parameter maps themselves are clipped correctly to the basin. So that is a small thing that is nice to address

Still I'll approve it and nice that it can be included in the release!

@hboisgon hboisgon merged commit c497316 into main Jan 29, 2025
4 checks passed
@hboisgon hboisgon deleted the landuse_vector branch January 29, 2025 07:28
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