-
Notifications
You must be signed in to change notification settings - Fork 7
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 ch and canfrac #109
Conversation
@angehung5 great start thanks! As discussed, will wait until all code/script/file changes have been made and the new variables are consistent for canopy height (ch) and canopy fraction (canfrac) instead of "forest" only related names/variables. This will also include changes to input canopy thresholds controlled in namelist, fch_thresh and frt_thresh, which should become something like "ch_thres" and "cf_thresh", respectively. This may impact CI and testing features, @zmoon ? |
@drnimbusrain @zmoon OK, I changed "fh/fch", "ffrac", "fch_thresh", and "frt_thresh" to "ch", "canfrac", "ch_thresh", and "cf_thresh", respectively. Code checks passed and there is no old var names in the code, but please check again in case I miss anything. |
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.
Looks good @angehung5 , and I think this covers all instances. I am uploading the new AWS files now. Can you run a test on the default slurm script for the global data process and run to make sure all works and looks OK for both scripts and canopy-app run and outputs?
@angehung5 Also, since there are NL changes to the thresholds, as mentioned I think there also needs to be changed in Zach's example jupyter notebook: https://github.com/angehung5/canopy-app/blob/main/python/examples.ipynb |
Thanks @drnimbusrain I see that although for 30eea0b there was an error when running the nb, it didn't fail CI. I should fix that. Probably need to check the rc in the loop and exit if nonzero. |
@zmoon Thanks for checking this! Are we good to go for now though as @angehung5 made the changes to the examples nb, or do you want to address that CI check here? |
Updated code works fine with southeast US files, and the slurm script and global python script work as well. Outputs look good. |
Thanks @angehung5 |
Based on #106
Example files are updated:
README data table is also updated but I leave the thresholds to you.