-
Notifications
You must be signed in to change notification settings - Fork 15
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
Link LAI and landuse classes #273
Conversation
@JoostBuitink do you have any preference for the last items in the TODO list? |
Hi Hélène! I was taking a look at the naming of the functions in the workflows. Now there is a |
I was also wondering. Did you perhaps do a check with such a table where you generated LULC and LAI maps, created a mapping table (with different resampling methods) to create new LAI maps? And compare those to the original LAI maps to see how they perform? I think that this would be a nice check. I could also do this myself for some models but that may take some time and delay the implementation of this PR due to my travels next week ;) |
I agree, it's a good suggestion. It's workflows name so they are not used often by users but create_lulc_lai_mapping_table sounds clearer |
I do test creating for piave with lai from modis and the mapped lai. From the tests, you can already see that depending on the sampling method, you get quite different values in the mapping table. I dont think we really need to test for this PR, also not sure how sensitive wflow models are to LAI and if the small differences with modis or the mapping table will impact the model much. I think this will something for users/projects to figure out and get experience with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished the review, looks good! I already saw your replies to my comments and I agree. So I suggest some minor changes for readability of the code and documentation and then it is good to go
used. This method is less precise as for cells with a lot of different | ||
landuse classes, the most frequent value might still be only a small | ||
fraction of the cell. More landuse classes should however be covered and | ||
it can always be used with the landuse map of the wflow model instead of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option is mentioned here to use the landuse map of the Wflow model. This option does not appear in setup_lai_from_lulc_mapping
, at least not explicitly (one could load the landuse map as a DataArray and pass it to the method). Is that something to add as a functionality to the method? Or remove the remark here if it is not implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone derives the landuse maps for wflow, he/she will likely use setup_lulc
to generate the map. And in that case, it would make more sense to use the original lulc data than using the staticmap itself. Or am I maybe missing a specific use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! Well you can always add the wflow staticmaps to your data catalog and I can imagine when you create a landuse scenario (eg crops into forest), you can update the original landuse map or decide to copy and update the wflow landuse map. I think it's always better to modify the original data but you never know.
hydromt_wflow/workflows/landuse.py
Outdated
def lulc_lai_mapping( | ||
da_lulc: xr.DataArray, | ||
da_lai: xr.DataArray, | ||
resampling_method: str = "mode", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we allow the user to specify a resampling method but in the landuse
workflow the methods are fixed by a global variable RESAMPLING. I was wondering, would it be nice to include the option for resampling methods in the land use workflow for the user, similar to this? (Not as part of this PR) Or is there a good reason that it is fixed for that workflowbut not in this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this not the resampling method of the landuse map but more how to sample lai values for each landuse class. Modis is lower resolution so you may have several landuse classes in one MODIS/LAI values. So you can decide to use that LAI values if only all LU cells are the same class, the majority or any. I can maybe rename to sampling method to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check, clear!
Issue addressed
Fixes #243
Explanation
See issue.
For deriving LAI values, I used several methods:
TODO
Checklist
main
Additional Notes (optional)