-
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
ILAMB task #197
ILAMB task #197
Conversation
Update Mar 17, 2022:
|
Hey @forsyth2 , this PR is ready for review. The example cfg to test a run can be found here:#197 (comment) |
@chengzhuzhang Thanks! I will review and test. I also created #229 to support the other machines. |
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.
From what I can tell, the code looks mostly alright -- I just made a few comments.
I will test your configuration file and run the unit/integration tests on Chrysalis once it's back online.
os.path.join( | ||
scriptDir, | ||
"ts_%s_%04d-%04d-%04d.status" | ||
% ("land_monthly", c["year1"], c["year2"], c["ts_num_years"]), |
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.
Is "land_monthly" always going to be the subsection?
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.
I meant to have "land_monthly" the minimum dependency. One can have "atm_monthly" data processed, but not required.
scriptDir, | ||
"ts_%s_%04d-%04d-%04d.status" | ||
% ( | ||
"atm_monthly_180x360_aave", |
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.
Is "atm_monthly_180x360_aave" always going to be the subsection?
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.
no, same comment as above..
fi | ||
|
||
# Point to obsersvation data | ||
# TODO: need to update these data to other supported machines |
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.
Note: Will be done in #229
echo ${workdir} | ||
echo {{ scriptDir }} | ||
|
||
srun --export=ALL -N 1 ilamb-run --config ilamb_run.cfg --model_root $model_root --regions global --model_year ${Y1} 2000 |
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.
I think you could just put export="ALL"
in your submitScript
call rather than running srun
here.
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, it makes sense! maybe this can be handled in #229 as well?
|
||
# Run diagnostics | ||
# Note --export=All is needed to make sure the executable is copied and executed on the nodes. | ||
# TODO: find the mpi run format for different platforms |
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.
Note: Will be done in #229.
echo 'ERROR (2)' > {{ prefix }}.status | ||
exit 2 |
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.
Since this is the first exit, let's do ERROR (1)
and exit 1
.
echo ===== COPY FILES TO WEB SERVER ===== | ||
echo | ||
|
||
# TODO copy _build from $workdir to web server |
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.
Is the output copied to the web server in another piece of code somewhere else?
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.
No. The output is still in workdir.
Now that Chrysalis is back up, I ran your configuration file and the unit/integration tests: This is my version of your configuration file:
That causes The unit tests need to be updated as do the expected files for the integration tests. |
@chengzhuzhang Since this branch is off the main repo rather than a fork, I think it should be easy for me to push further changes to it. Would you like me to make the fixes I suggested? |
Yes, please! I'm also not familiar with zppy's tests, please go ahead and make the fixes for me. Thank you! |
@chengzhuzhang Do you know why I got |
Those are ilamb error logs tell that some expected variables are not located. The tested configuration only included a minimal sets variables. So 'ILAMB.ilamblib.VarNotInModel: VarNotInModel' is expected. did your run actually create a index.html with some results? |
Ah ok. I moved my results to https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_development/pr_197/_build/. Most of it is missing data except for some leaf-area-index values, which matches the variables given, so I think that works then. |
Closed with #230. |
This PR is to add capability to run ILAMB package for land model bench-marking. Close #133, #134.
Example configuration file:
External development for e3sm_to_cmip
'mrsos, mrso, mrfso, mrros, mrro, prveg, evspsblveg, evspsblsoi, tran, tsl, lai, cLitter, cProduct, cSoilFast,cSoilMedium,cSoilSlow fFire, fHarvest, cVeg, nbp, gpp, ra, rh'
Conversion formula should be added here: https://acme-climate.atlassian.net/wiki/spaces/DOC/pages/925500501/Lmon+variable+conversion+table
ILAMB has some underlying assumptions and are noted here (Thanks to @minxu74, one of the ILAMB developers, to help me understand):