EAMxx: extract diagnostic creation logic outside of output class #6796
Conversation
This GM is useful for unit tests, but more generally for situations where only grids are avaialble, but interfaces require a grids manager.
bartgol
added
BFB
|
bartgol
force-pushed
the
bartgol/eamxx/encapsulate-diagnostics-creation
branch
2 times, most recently
from
7710d70 to
52ae29f
Compare
* Add free function to build a diagnostic obj from a diag field name * Adapt existing diags so that their name is "static", rather than matching the diag field name * Adapt AtmosphereOutput to use this utility. Enhances encapsulation, and avoids hiding the diag naming convention deep in the IO class
bartgol
force-pushed
the
bartgol/eamxx/encapsulate-diagnostics-creation
branch
from
52ae29f to
a851099
Compare
| // Construct a diagnostic by this name | ||
| ekat::ParameterList params; | ||
| std::string diag_name; | ||
| // Some diags need some extra setup or trigger extra behaviors |
There was a problem hiding this comment.
Not clear to me what the FieldAtXyz diag needs this special-casing ... consider adding an explicit comment?
There was a problem hiding this comment.
I think the gh diff may make it hard to see, with the red lines intertwined with the green lines. If you look at the file, it should be clear, since it's right there: we possibly set the mast value in the params list, and we trigger avg count.
Now, why we do those things, that may indeed not be obvious, so I'll add a comment.
There was a problem hiding this comment.
Yep, why do we need to trigger the avg counts, etc. for only the FieldAtXyz ones? That was my confusion... Is there something special?
There was a problem hiding this comment.
Yeah, FieldAtHeight and FieldAtPressure may request to find the value outside what's actually in the model (e.g., FieldAtPressure for 1000mbar is OOB on mount Everest), which generates FillValue entries in the diag. When we do averaging, we don't want to count the FillValue entries, so we keep track of how many time samples are actuall valid (and we keep a separate count for each column), so that we can only average those.
There was a problem hiding this comment.
Well, shouldn't we let all diags do that btw? AodVis does assign fillvals
There was a problem hiding this comment.
Not all diags create fill value. If Aodvis does, then yes, we should handle that too. We could maybe think of adding a method to AtmosphereDiagnostic, something like "bool can_generate_fill_value()" so that we don't have to do all these if/else...
There was a problem hiding this comment.
Ok, since I was responsible for adding those, I can handle them in a subsequent PR. For the record, both atm_backtend and aodvis use fill_value. For aodvis, to fill nighttime indices. For atm_backtend, for undefined behavior in the first step
|
@bartgol are you happy with the state of this? I can take care of the other diags needing the fillval treatment in subsequent PRs (no need to address that now). If so you could merge at your convenience :) |
Extract creation of diagnostic classes out of the AtmosphereOutput class and into its own routine.
This allows to
To avoid issues like the ones discussed in this conversation, we now use std::regex. The regexes look for special strings (such at "at_500mb") at the end of the diagnostic field name. This ensures that we pipe diagnostics correclty, and process them one at a time. In particular, parsing somethin like
blah_at_500hPa_atm_backtendis univokely parsed as "the back-tendency of the fieldblah_at_500hPa", the latter being itself a diagnostic to be created once the output class process the "outer" diagnostic inputs.Side note: it is different to request
T_mid_at_500hPa_atm_backtendvsT_mid_atm_backtend_at_500hPa, since the two diags are nested in specular ways. In the end, the result should be similar (up to roundings/truncations), but computationally speaking, one of the two may be cheaper than the other.