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

Update USLE C parameter with planted forest and orchard values #234

Merged
merged 11 commits into from
May 7, 2024

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Jan 31, 2024

Issue addressed

No issues opened. By default forest USLE C (erosion) is very small 0.001 while in literature, planted forest is 0.0881 and orchard even 0.2188. Shapefile data from Global Forest Watch is available to refine the landuse map with planted forest so this can be use to get better USLE C values. This PR adresses this.

Explanation

Burn planted forest and orchard in the USLE C and update values.

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 self-assigned this Feb 15, 2024
@hboisgon hboisgon added this to the 0.6 release milestone Apr 8, 2024
Copy link
Collaborator

@dalmijn dalmijn left a comment

Choose a reason for hiding this comment

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

@hboisgon, very nice work! Nice to update the typing in the WflowSedimentModel methods as well. I only have some very minor comments.

Comment on lines 451 to 452
if not isfile(fn_map) and fn_map not in self.data_catalog:
raise ValueError(f"Riverbed sediment mapping file not found: {fn_map}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this, this is caught by the DataCatalog right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If so, this could then also be removed from the WflowModel object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. We often forget to update wflow_sediment with new changes so with this PR, I already chnaged what I saw including adding typing. I missed this one and now removed the line in wflow_sediment.

I did not find the place you meant in WflowModel object though... Thought I removed all those in a specific PR already?

Copy link
Collaborator

@dalmijn dalmijn Apr 29, 2024

Choose a reason for hiding this comment

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

Line 907 in setup_lulc_maps

hydromt_wflow/wflow_sediment.py Outdated Show resolved Hide resolved
tests/test_model_methods.py Show resolved Hide resolved
@@ -15,6 +15,7 @@ Added
- New setup method for the **KsatHorFrac** parameter **setup_ksathorfarc** to up-downscale existing ksathorfrac maps. `PR #255 <https://github.com/Deltares/hydromt_wflow/pull/255>`_
- new function **setup_pet_forcing** to reproject existing pet data rather than computing from other meteo data. PR #257
- Workflow to compute brooks corey c for the wflow layers based on soilgrids data, soilgrids_brooks_corey. PR #242
- **setup_lulcmaps** for wflow_sediment: if planted forest data is available, it can be used to update the values of the USLE C parameter. PR #234
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- **setup_lulcmaps** for wflow_sediment: if planted forest data is available, it can be used to update the values of the USLE C parameter. PR #234
- **setup_lulcmaps** for wflow_sediment: if planted forest data is available, it can be used to update the values of the USLE C parameter. `PR #234 <https://github.com/Deltares/hydromt_wflow/pull/234>`_

PR link? I notice that we sometimes do and sometimes don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we should be consistent but in hydromt we only put the PR number and not the link anymore. Let's do that in the release PR.

There are two pages where the changes appear:

  • release tag: there if you just put the PR number the link appear automatically but the rst link really does not.
  • changelog doc: there the links do not appear.

What do you think is best? I think for the release, we generate the note automatically from the changelog so I don't know if we can update the links easily unless we decide to generate it manually again? If not then we should just put the Pr number and not the link.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The site we can regenerate. The links in the release when can just edit afterwards. But maybe it becomes a bit of a hassle so we could opt to leave the link out.

hydromt_wflow/wflow_sediment.py Show resolved Hide resolved
hydromt_wflow/wflow_sediment.py Show resolved Hide resolved
@hboisgon hboisgon requested a review from dalmijn April 29, 2024 03:19
Copy link
Collaborator

@dalmijn dalmijn left a comment

Choose a reason for hiding this comment

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

If the data catalog check in setup_lulc_maps is removed from WflowModel then it LGTM.

@hboisgon hboisgon merged commit f77c3c0 into main May 7, 2024
6 checks passed
@hboisgon hboisgon deleted the uslec_forest branch May 7, 2024 03:25
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