eamxx: set fillval thresholds to match science/diags#8481
Conversation
mahf708
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adjusts two threshold constants in EAMxx that control when data is treated as valid vs
masked/fill-valued during horizontal remapping and output averaging, to better align behavior
with science/diagnostic expectations.
Changes:
- Update the real-mask cutoff used when rescaling masked fields during horizontal remapping.
- Lower the default averaged-output “valid sample” threshold used to decide when to emit FillValue.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
components/eamxx/src/share/remap/horizontal_remapper.cpp |
Changes the hard cutoff used to decide whether to rescale vs fill masked target entries during remap. |
components/eamxx/src/share/io/scorpio_output.hpp |
Lowers the default averaging validity threshold that influences FillValue propagation in averaged output. |
| const int rank = layout.rank(); | ||
| const int ncols = m_tgt_grid->get_num_local_dofs(); | ||
| const Real mask_threshold = std::numeric_limits<Real>::epsilon(); // TODO: Should we not hardcode the threshold for simply masking out the column. | ||
| const Real mask_threshold = 0.25; |
| Real m_avg_coeff_threshold = | ||
| 0.5; // % of unfilled values required to not just assign value as FillValue | ||
| 0.01; // % of unfilled values required to not just assign value as FillValue |
bartgol
left a comment
bartgol
left a comment
There was a problem hiding this comment.
I agree with the direction of the change. As for the actual numbers, I think that may be somewhat up for debate.
Two comments:
- I wonder if using the same % threshold for remappers and averaging would be better. The reason being, if you are fine with 1% of valid time snapshots, why aren't you fine with 1% of the (spatial) patch being valid and ask for 25% instead?
- To avoid having to change hard-coded values again, we should consider making the remapper threshold configurable, and add another parameter in the output yaml to tune it (AtmosphereOutput will have to pass the param to the horiz remapper when constructing it).
These are not points on which I feel strongly, so I'm approving. More like thoughts. I'd like to hear what you think about them though.
Let me think, I am not very happy that we are moving out from the infinitesimal setup. To me, that's theoretically sound. I wonder if something else is amiss. Lemme take a deeper look. I'd like to keep these two thresholds defaulted to std::numeric_limits::epsilon() |
This pull request makes targeted adjustments to threshold values in the EAMxx component, specifically affecting how masked and unfilled data are handled in output and remapping routines. These changes aim to improve the robustness and accuracy of data processing by fine-tuning when data is considered valid or should be masked.
Threshold adjustments for data validity and masking:
m_avg_coeff_thresholdinAtmosphereOutputfrom0.5to0.01, making the output more tolerant of unfilled values before assigning the fill value. (components/eamxx/src/share/io/scorpio_output.hpp)mask_thresholdinrescale_masked_fieldsfromstd::numeric_limits<Real>::epsilon()to0.25, setting a more practical threshold for masking out columns during remapping. (components/eamxx/src/share/remap/horizontal_remapper.cpp)