Repository navigation
New YAML schema - #119
New YAML schema#119AgentOxygen wants to merge 9 commits into
Conversation
maritsandstad
left a comment
maritsandstad
left a comment
There was a problem hiding this comment.
So, I think this looks like a really good way to go, and really amazing refactoring.
However, I am getting a slight bit of whiplash from it, and think we might need a bit more time and validation to check that this rewrite isn't loosing us something important that we have tested before, but we don't have test coverage for (which I think might be a significant amount). Maybe you two @AgentOxygen and @mvertens are completely on top of this, and it might be fine, at least for my part I don't feel like I have a grasp of it that is through enough that I want to merge it in directly, especially given that I think we will start doing production runs fairly soon (hopefully).
| grids: | ||
| description: >- | ||
| Default output grids for every entry in this file: gn keeps the native | ||
| grid, gr regrids to lat/lon. Default [gr]. | ||
| type: array | ||
| minItems: 1 | ||
| uniqueItems: true | ||
| items: {enum: [gn, gr]} |
There was a problem hiding this comment.
Flagging; we will want to change this to the updated CMOR-table defined grids for CESM and NorESM as defined in the EMD
That's totally fair. Especially since we are expecting to start production runs soon, it would be safer to leave this in draft form until we have implemented more test coverage separately. The main reason I ended up with a large refactor was because writing a schema for the existing structure was terribly verbose and added more complexity than it was worth. For now I can focus on just extending the existing test suite as I develop our new functions. We can hopefully revisit this after testing coverage is improved. |
|
@AgentOxygen - this is great! I had claude review the yaml files - here is a summary of the issues that were found. @marit @AgentOxygen - I think this refactor has actually exposed some real problems that need to be addressed and I am wondering if we should migrate it out of draft form once we resolve these issues. A meeting this Friday would be helpful. Mapping entries that don't match the CMOR tables54 entries across the Checked against the Background: how a name is builtA CMIP7 variable name has a short root name, then a suffix of codes: The root says what the quantity is. The suffix says how it was sampled. The The area code is the one that matters below. Why a mismatch mattersThe mapping files now hold one line per variable and look up everything else The search is on the full name, as one string. If the name isn't found, nothing Regridding is not affected by any of this. The conservative-or-bilinear choice The four kinds of mismatch
Only B is fixed in code. A, C and D need someone to decide what was intended. A. No matching name (2)Both are CESM relative humidity. The atmos table has
Someone needs to say which height these were meant to be at. B. Right name, wrong table (19)The name is spelled correctly and exists in CMIP7 -- just in a different The groupings are sensible ones. Irrigation fields belong in the CESM atmos
The fix is for That file's name is also the answer to which realm the variable belongs to, so One rule still has to be decided: what to do if a name turns up in more than C. Not in CMIP7 at all (19)For these, every table was searched for the root name on its own, ignoring the Four of them (
D. Two entries, one of which matches (14)These are seven quantities, each appearing twice in both sea-ice files. One of
siconc_tavg-u-hxy-si: # not in the table
formula: {day: siconc_d, mon: siconc}
units: '%' # units written out by hand
siconc_tavg-u-hxy-u: # in the table
formula: {day: siconc_d, mon: siconc}
# no units; taken from the tableThe entry that doesn't match the table has its units written out in the file. So this is not a set of typos, and renaming the unmatched entry would collide
Each row appears in both The remaining two of the 14 are How this was checkedFor each mapping file, take the variable names and look each one up in
Step 1 is the one that matters. Without it, the 14 entries in category D look |
|
So this is all super useful, and I don't think it necessarily needs to stay in draft, just we want to do some validation and testing, also possibly discuss how we think about the direction and implication for our csv to yaml setup (I think a lot might be superfluous in the current code that we added to conform to the old yaml scheme). One question I have for instance is what is expected for the unit conversion. To me it looks like the intention to work towards doing it automatically from comparing inputs to requests? If so, I'm just concerned whether that this doesn't cause some sort of double conversion (for NorESM we've generally gotten scientists to make those types of conversions explicitly by writing in formulas). |
| dataset_overrides: | ||
| institution_id: NCC | ||
| nominal_resolution: 200 km | ||
| source_id: NorESM3 |
There was a problem hiding this comment.
So, one more flag, in this rewrite, where does this information live?
There was a problem hiding this comment.
Sorry, actually really good to get this out from here, it is not used anyways. Looking currently at how to change so we can use it with updated tables version.
|
@AgentOxygen - I have merged this to main and resolved conflicts - and I also needed to add a change to cmor_utils.py to added import of lru_cache. Can I push back to your branch at this point? @maritsandstad - I ran run_reference_case in my sandbox (~/test_cmor is /scratch/mvertens/noresm3/test_cmor/): and the validation summary is here: If the validation looks reasonable I propose that I push back to the branch origin/yaml-schema and merge it. @AgentOxygen @maritsandstad - does that sound reasonable? |
|
@mvertens , yes this sound reasonable to me. However, do you have a link to a summary without this? I am mainly concerned with whether this produces changes to what we are able to produce. |
|
@maritsandstad - the original validation reports are in /projects/NS9560K/www/diagnostics/noresm/n1850GaxgGHG.LM.nor30b25.528.20260928.main |
|
The original validation report can be found at |
|
I did not have plots for the original - so I think I need to regenerate them to have a valid comparison. |
|
No worries, the original definitely does not do any better. I'm fine with the validation here, we need to get people to check things anyway, this doesn't look to be breaking anything to me. |
|
@maritsandstad @AgentOxygen - in looking at the validation reports and comparing the plots it looks like this PR is showing up more plots than what we get from main: |
|
I am happy for conflicts to be resolved and this to be merged in, and then I think we need to have a coordinated validation round with the scientists on our end again soon anyways @mvertens |
3 participants
This is a large change that needs careful revision and feedback on before merging.
A centrally defined schema for all of our YAML files. This makes many of the assumptions made throughout the source code explicit and allows us to enforce them when we inevitably change the YAMLs to accommodate updates to our upstream spreadsheets.
The new schema is defined in one file using JSON-Schema, which is added as a dependency:
data/schemas/mapping.schema.yamlA lot of the entries in the YAML files were unused or otherwise redundant and could be replaced with reasonable defaults. This greatly reduced the size and complexity of the YAML files. For many variables, a one-line rename mapping is all that is needed, so entries take on either a "short form" or "long form" style.
Short form example (simple rename):
Long form example:
Changes from the old format:
formulacan be delineated between daily and monthly in the same blocksourcesnow derives from the formula directly rather than restatingtabletable gets set by the specifiedrealmparameterunitssets formula result units if they differ from CMIP7 tablelevelscomes from the CMIP7 table entry dimensionsregrid_methodassumes conservative, but overrides come fromdata/intensive_vars.yamlcell_methodsderived from CMIP7 tablelong_namederived from CMIP7 tablestandard_namederived from CMIP7 tableThis is a minimal implementation in the interest of avoiding large PRs. As a result, I suppressed the linter rule for too many lines. This should be removed in a later clean up.