-
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
Leaf Age Response function for all Biogenic VOCs consistent with MEGAN #92
Conversation
@quaz115 There are build errors in your codes, as well as formatting issues too. Please make sure you are following the "develop" practices in the README, using things like the pre-commit hooks to check indentation etc. |
@zmoon Even though i made a new conda env which has pre-commit installed, seems like findent isn't working ?
|
@quaz115 I tested your repo/branch and using the pre-commit run --all-files on GMU Hopper system and it works OK, and finds wfindent:
Of course here I am using a later version of python3.10 in my install of pre-commit, compared to your version of python 3.9 installed in your environment. Maybe try installing pre-commit again, but with later version of python? conda install -c conda-forge pre-commit python=3.10.5 |
Eventually, when merged this will close #79 |
Had to use 3.11 to make findent work, also resolving other build errros now |
@drnimbusrain It passes all checks and compiles now, but will prefer to merge only after run(s) with this new functionality. Also, let me know when you have reviewed the code changes once and have free time to discuss them sometime early this week. |
Yes, please let us review the code carefully, and it would be best to show
tests and results here before merging.
I'd like it if Maggie cof help with that too
…On Sat, Sep 16, 2023, 5:00 PM Quazi Ziaur Rasool ***@***.***> wrote:
@drnimbusrain <https://github.com/drnimbusrain> It passes all checks and
compiles now, but will prefer to merge only after run(s) with this new
functionality. Also, let me know when you have reviewed the code changes
once and have free time to discuss them sometime early this week.
—
Reply to this email directly, view it on GitHub
<#92 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGLFYNVHA7MUCTB2FLGAQL3X2YHPFANCNFSM6AAAAAA4Y6U4SY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@MaggieMarvin Would be great if you could help provide a close code review here, as well as run some tests with for Quazi's PR/branch to gauge if it is working as expected and the impacts. Thanks! |
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.
@quaz115 Finished my initial review, and looks pretty good. My major comment is that you set the above canopy air temperature to the canopy resolved leaf temperature. I think this should be changed to use the more appropriate 1D/2D 2-meter temperature from model/obs inputs. This seems more relevant, and then the TABOVECANOPY
variable does not need to be an array indexed on canopy model levels (modlays, ZK, etc.).
Once updated for this, @MaggieMarvin could review and test the impacts of leaf age factors.
Nice work!
@drnimbusrain @MaggieMarvin I have edited the code (compiles as well) based on Patrick's recommendation to pass TEMP2 (above 2-m air temp) for GAMMA_LEAFAGE. |
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, but some other small comments and changes.
…n canopy_utils_mod.F90 in the GET_GAMMA_LEAFAGE function
Looks good Quazi, before approving, I'd like it if @MaggieMarvin do a test with the impacts of turning the leaf age factor on and off for isoprene. Thanks! |
@angehung5 Can you make another PR to @quaz115 's fork of develop, with your updated SE text/nc files(with same daily LAI and pavd updates as before) and updated python script for daily LAI merge from global AWS canopy/GFS files? This will facilitate @quaz115 testing of daily LAI in his develop branch for the leaf age PR. |
@drnimbusrain Attaching the SE US output with @angehung5 's new daily LAI inputs showing difference b/w LEAFAGE OFF vs ON: |
@drnimbusrain @zmoon @MaggieMarvin @angehung5 Feel free to review this, tested with the SE US setup and see if its ready to be merged currently |
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.
@quaz115 Thank you...Some small clarifications and changes.
@quaz115 Can you comment on these results a bit as to what is being shown, impacts, and if expected results? |
…in namelist.canopy Co-authored-by: Patrick Campbell <dr.nimbusrain@gmail.com>
Co-authored-by: Patrick Campbell <dr.nimbusrain@gmail.com>
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.
Be sure to run the CI first, seems some small formatting issue with your change: https://github.com/noaa-oar-arl/canopy-app/actions/runs/6776276783/job/18417393784
…opts_mod.F90 Co-authored-by: Patrick Campbell <dr.nimbusrain@gmail.com>
…on corrected in canopy_canopts_mod and REmove print diags
@zmoon Can you tell why the CI is failing here? |
@drnimbusrain Normally leaf age response would have a reduction on BVOCs emission rate except for Higher Monoterpene emissions in some cases possibly due to new leaf response as a defense mechanism giving high MT emissions: [Guenther et al. 1991 Also, Guenther et al. 2012: "Table4 shows that new leaves are predicted to have just 5% of the isoprene emission rate, but a factor of 3.5 higher methanol emission rate, in comparison to mature leaves. Monoterpene emissions are predicted to decline with age while sesquiterpene emissions are predicted to increase." So most likely we are seeing this as Fnew and Fgro being higher for these results as a possible reason |
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.
@quaz115 small namelist changes, other than that looks good.
input/namelist.canopy
Outdated
! file_vars = 'input/gfs.t12z.20220630.sfcf023.canopy.txt' 'input/gfs.t12z.20220701.sfcf000.canopy.txt' 'input/gfs.t12z.20220701.sfcf001.canopy.txt' | ||
file_out = 'output/2022-07-01-11-0000_southeast_us' | ||
file_out = 'output/2022-07-01-11-0000_global_leafageON' !southeast_us' |
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.
file_out = 'output/2022-07-01-11-0000_global_leafageON' !southeast_us' | |
file_out = 'output/2022-07-01-11-0000_southeast_us' |
input/namelist.canopy
Outdated
@@ -17,8 +20,8 @@ | |||
time_end = '2022-07-01-13:00:00.0000' | |||
ntime = 3 | |||
time_intvl = 3600 | |||
nlat = 43 | |||
nlon = 86 | |||
nlat = 43 !for Global nlat= 1536 |
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.
nlat = 43 !for Global nlat= 1536 | |
nlat = 43 |
input/namelist.canopy
Outdated
nlat = 43 | ||
nlon = 86 | ||
nlat = 43 !for Global nlat= 1536 | ||
nlon = 86 !for Global nlon= 3072 |
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.
nlon = 86 !for Global nlon= 3072 | |
nlon = 86 |
input/namelist.canopy
Outdated
@@ -48,6 +51,8 @@ | |||
crop_set = 3.0 | |||
co2_opt = 0 | |||
co2_set = 400.0 | |||
lai_tstep = 86400 | |||
leafage_opt = 1 |
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.
leafage_opt = 1 | |
leafage_opt = 1 |
input/namelist.canopy
Outdated
@@ -1,9 +1,12 @@ | |||
&FILENAMES | |||
!2D Text and NCF Examples | |||
! Recommend set file_out prefix to initial 'YYYY-MM-DD-HH-MMSS_region_identifier' | |||
file_vars = 'input/gfs.t12z.20220630.sfcf023.canopy.nc' 'input/gfs.t12z.20220701.sfcf000.canopy.nc' 'input/gfs.t12z.20220701.sfcf001.canopy.nc' | |||
! file_vars = 'input/gfs.t12z.20220630.sfcf023.canopy.nc' 'input/gfs.t12z.20220701.sfcf000.canopy.nc' 'input/gfs.t12z.20220701.sfcf001.canopy.nc' |
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.
@quaz115 Please also revert these input files back to what they are in default [develop] branch. Thanks!
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.
this should be uncommented right: file_vars = 'input/gfs.t12z.20220630.sfcf023.canopy.nc' 'input/gfs.t12z.20220701.sfcf000.canopy.nc' 'input/gfs.t12z.20220701.sfcf001.canopy.nc'
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.
Should look like this:
&FILENAMES
!2D Text and NCF Examples
! Recommend set file_out prefix to initial 'YYYY-MM-DD-HH-MMSS_region_identifier'
file_vars = 'input/gfs.t12z.20220630.sfcf023.canopy.nc' 'input/gfs.t12z.20220701.sfcf000.canopy.nc' 'input/gfs.t12z.20220701.sfcf001.canopy.nc'
! file_vars = 'input/gfs.t12z.20220630.sfcf023.canopy.txt' 'input/gfs.t12z.20220701.sfcf000.canopy.txt' 'input/gfs.t12z.20220701.sfcf001.canopy.txt'
file_out = 'output/2022-07-01-11-0000_southeast_us'
See Issue #79 .
@drnimbusrain @MaggieMarvin These are the changes I have added so far for the Leaf Age response function to work for all BVOCs.
Think we need more discussion on how to make it work with the time feature (as I have currently tried to make tdays, i.e. time period between Past and Current LAI (whichever time period we decide to use) to be dynamic and user-defined. This si the only component (LAI and timestep inputs) that would need more work.
@drnimbusrain feel free to point out any further changes I need to make before we can finalize this. I have made this a draft PR currently, but it is almost complete I think
Let me know if you have questions about specific changes