-
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
Remove cdscan for e3sm_diags #598
Conversation
@@ -101,19 +101,9 @@ create_links_ts() | |||
YYYY=`printf "%04d" ${year}` | |||
for file in ${ts_dir_source}/${v}_${YYYY}*.nc | |||
do | |||
# Add this time series file to the list of files for cdscan to use | |||
echo ${file} >> ${v}_files.txt | |||
cp ${file} ${ts_dir_destination}/${v}_${YYYY}*.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.
I suppose ${ts_dir_destination}/
isn't strictly necessary, since we have cd ${ts_dir_destination}
above.
@tomvothecoder Re: #519 (comment) -- this PR includes the changes that couldn't be included in #519 since the |
607b728
to
bfb0ed7
Compare
bfb0ed7
to
d88734e
Compare
@tomvothecoder I created a new "min case" cfg for testing sets that have already been migrated. They appear to generate alright using E3SM Diags built off the latest CDAT branch: https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_cdat_migrated_www/issue-346-diags-20240802/v3.LR.historical_0051/e3sm_diags/ |
I ran The cfg
I specifically set:
to test the latest sets added on https://github.com/E3SM-Project/e3sm_diags/commits/cdat-migration-fy24 (apparently missing These three sets unfortunately ran into some errors. https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_cdat_migrated_www/test-diags-no-cdat-20240917/v3.LR.historical_0051/e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1987-1988/viewer/ only shows
This error matches the one on
The |
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_cdat_migrated_www/test-diags-no-cdat-20240917/v3.LR.historical_0051/e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1987-1988/viewer/ shows only The relevant output directory can be seen here, and is also mentioned in a code block above.
Note https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_e3sm_diags_cdat_migrated_www/test-diags-no-cdat-20240917/v3.LR.historical_0051/e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1987-1988/viewer/ also shows only |
@forsyth2 I'm looking at the log files (as below), it appears that time series files are not correctly feed into e3sm_diags, which means the code change that removes
|
Thanks @chengzhuzhang. I haven't had problems with previous added sets though. I'll try to debug this further to see if it's something with how this PR handles these sets in particular... |
@tomvothecoder we will need to coordinate on this issue with e3sm_diags refactoring. The original cdscan codes basically subset and extract the metadata from input file for the specified time period and generate an In the new |
Could you clarify "the previous added sets", i think the issue only relevant for time-series based diagnostics sets. For sets depends on climo datasets, they should be fine. |
@forsyth2 I saw the test results from Aug 6, yes by then those sets only rely on climo files. |
@chengzhuzhang Does this mean the bug is for certain in the Diags code or should I dive deeper into the zppy changes in this PR? |
It is not a bug in diags. We can also add the time selecting and slicing code in |
To summarize this issue, based on what I'm reading:
I will open a PR to update e3sm_diags to use
The time slicing and subsetting code you listed parses the filepath to get the start and end years. I'm not sure if this currently works with a directory specified as an input path. |
@forsyth2 Can you provide the input paths for the test data? I am trying to reproduce your errors by running prov/e3sm.py but the |
Note, opening multiple time series files is actually supported already if deriving from other variables. If we are trying to open up multiple datasets for the same variable (e.g., split up by months or something) without deriving, then we need to add it in #861. |
Everything is specified in the cfg I have copied in #598 (comment). Specifically:
For the
|
TODO:
|
There have been a number of code changes since original work on this; I started fresh with #651, so closing this PR. |
Resolves #346. Resolves #80. Follow-up to #519.