-
Notifications
You must be signed in to change notification settings - Fork 124
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
Refactoring adding support for custom projects #195
Refactoring adding support for custom projects #195
Conversation
Looks fine to me. |
esmvaltool/config-developer.yml
Outdated
|
||
CMIP5: | ||
cmor_type: 'CMIP5' |
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 needs to be also added to other projects based on the CMIP5-CMOR tables.
I get an error when running namelist_perfmetrics_CMIP5.yml
File "/home/righ_ma/ESMValTool/public/esmvaltool/main.py", line 259, in run
main()
File "/home/righ_ma/ESMValTool/public/esmvaltool/main.py", line 199, in main
process_namelist(namelist_file=namelist_file, config_user=cfg)
File "/home/righ_ma/ESMValTool/public/esmvaltool/main.py", line 235, in process_namelist
namelist = read_namelist_file(namelist_file, config_user)
File "/home/righ_ma/ESMValTool/public/esmvaltool/namelist.py", line 38, in read_namelist_file
raw_namelist, config_user, initialize_tasks, namelist_file=filename)
File "/home/righ_ma/ESMValTool/public/esmvaltool/namelist.py", line 436, in __init__
raw_namelist['diagnostics'])
File "/home/righ_ma/ESMValTool/public/esmvaltool/namelist.py", line 453, in _initialize_diagnostics
name, raw_diagnostic.get('variables'), models)
File "/home/righ_ma/ESMValTool/public/esmvaltool/namelist.py", line 517, in _initialize_variable_collection
self._initialize_variables(raw_variable, models)
File "/home/righ_ma/ESMValTool/public/esmvaltool/namelist.py", line 482, in _initialize_variables
_add_cmor_info(variable, cmor_keys)
File "/home/righ_ma/ESMValTool/public/esmvaltool/namelist.py", line 196, in _add_cmor_info
elif variable['cmor_table'] in CMOR_TABLES:
KeyError: 'cmor_table'
Yes. In fact, for CMIP5 is not needed because it defaults to the project name. I will fix it |
…e was not present in the variable config
Should be solved now. Please try it |
Approved 👍 |
input_file: '[var]_[mip]_[model]_[exp]_[ensemble]_[grid]_*.nc' | ||
output_file: '[project]_[model]_[mip]_[exp]_[ensemble]_[field]_[var]_[start_year]-[end_year]' | ||
cmor_type: 'CMIP6' | ||
cmor_tables: '~/PycharmProjects/earthdiagnostics/earthdiagnostics/cmor_tables/primavera' |
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.
Could we add those files to esmvaltool instead, just like the cmip5-cmor-tables
and cmip6-cmor-tables
? Or are they too big or otherwise restricted? I think it would be much more convenient for the user if she/he can just use the tables without having to search the internet for tables and installing those separately.
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.
We have a set of tables like this for nearly every project that shares model data. This will be a burden to maintain and the tool will grow too large.
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.
Ok, I still think this needs to be addressed, but maybe not in this pull request.
@@ -192,13 +192,19 @@ def _get_value(key, variables): | |||
def _add_cmor_info(variable, keys): |
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.
Wouldn't it be easier just keep this function more or less as it is, only replace the first two lines by, e.g.:
if variable.get('cmor_table') in CMOR_TABLES:
variable_info = CMOR_TABLES[variable['cmor_table']].get_variable(
?
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 was just to use variable['project'] as a default for variable['cmor_table'] to avoid having to specify it two times in most cases. But I think that the correct place to do this is at namelist reading level, so I will change it
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.
Done
esmvaltool/preprocessor/_reformat.py
Outdated
'CMIP6': CMIP6Info(), | ||
} | ||
|
||
def _read_cmor_tables(): |
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 this function should be in variable_info.py
and CMOR_TABLES
probably too and then we can import CMOR_TABLES
from there whereever it is needed. That also avoids reading the tables multiple times. And maybe the variable_info.py
file should be renamed to cmor_tables.py
or something more explanatory (or even better, create a cmor_tables
directory directly under esmvaltool
, i.e. one level up from interface_scripts
, which contains the cmor tables and the code for reading them, but maybe that is something for another pull request).
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 will move it
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 was already doing it!!!
I have added support for CMORized data that does not comply CMIP standards. First test was made with PRIMAVERA but this can be adapted to any project as long as they provide cmorized data and cmor tables in CMOR2 (CMIP5) or CMOR3 (CMIP6) format