Modify ensure_aggregatable_across_cases to ensure NetCDF cubes are correctly merged#1199
Modify ensure_aggregatable_across_cases to ensure NetCDF cubes are correctly merged#1199
ensure_aggregatable_across_cases to ensure NetCDF cubes are correctly merged#1199Conversation
|
|
jfrost-mo
force-pushed
the
merging_2d_time_coord
branch
from
a30a9fb to
5deec84
Compare
jfrost-mo
changed the title
ensure_aggregatable_across_cases to ensure NetCDF cubes are correctly merged.ensure_aggregatable_across_cases to ensure NetCDF cubes are correctly merged
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jfrost-mo
force-pushed
the
merging_2d_time_coord
branch
2 times, most recently
from
5cddad3 to
6f663c0
Compare
daflack
left a comment
daflack
left a comment
There was a problem hiding this comment.
From a run, there are positive signs of improvement with some LFRic and some difference plot aggregation occurring. However, there are some issues that require changes:
- aggregation by validity time - all attempts (regardless of model) fail due to lack of
timecoordinate - some fields are still being considered as cubelists therefore
slices_overfails this has particular problems for histogram aggregation - hour of day difference aggregation fails (difference operator needs a
timecoordinate - relates to #1204) - Lightning difference aggregation fails with `Attribute Error: "NoneType" object has no attribute "attributes"
|
Further notes on the errors - the second bullet point is not due to having both time processed fields and instantaneous fields in the output as the error around cubelists occur for fields that do not appear in the time processed files. |
jfrost-mo
force-pushed
the
merging_2d_time_coord
branch
from
a0a97ba to
65c519b
Compare
|
I think we now have a fix, or at least an explanation, for each of those failure cases. |
daflack
left a comment
daflack
left a comment
There was a problem hiding this comment.
Looks good to me and only fails with OOM - so happy for it to be merged once tests are a bit more comprehensive for the parts added.
|
Tests have now been added, so ready for final review. |
As the difference operator requires just two cubes this must be done beforehand. For MEAN collapsing, as these recipes do, this should give exactly the same result.
aggregate.ensure_aggregatable_across_cases has been modified to ensure it always tries to fix the provided cubes. To ensure cubes merge properly we also remove time_origin attribute from the time coordinate at load time, taking care to only remove it once we don't need it.
Remove collapse_by_lead_time as it can be replicated with collapse and remove unnecessary tests.
The only substantive change should be that we now remove the equalised_validity_time coordinate that is added during the operator.
We can only equalise the time coordinate when it actually exists, so we skip when it does not. Fixes #1204
It isn't useful to collapse over validity time for cubes that don't overlap, so now we provide a descriptive error message instead.
Test difference of cubes without time coordinates and error when trying to take difference of non-overlapping cubes.
jfrost-mo
force-pushed
the
merging_2d_time_coord
branch
from
6662849 to
2d75e08
Compare
daflack
left a comment
daflack
left a comment
There was a problem hiding this comment.
Looks good, a couple of questions around the tests. I'm not sure if you have deleted ones that we should still keep in and also wondering if there is an unnecessary if statement?
It significantly increased the testing burden and usage complexity of the operator for little benefit. It only affected whether to do a pre-collapse by lead time ahead of the coord categorisation, and we can automatically detect that.
jfrost-mo
left a comment
jfrost-mo
left a comment
There was a problem hiding this comment.
I've now ensured the tests cover everything changed in this PR.
As part of this I did remove the multi_case argument to collapse_by_hour_of_day. It significantly increased the complexity of testing, as well as general usage of the operator for very little benefit. I'm not convinced over any traceability benefits, as it only affects whether their is a collapse by lead time prior to categorising the coordinates, and the coordinates that are generated from that are removed within the operator.
daflack
left a comment
daflack
left a comment
There was a problem hiding this comment.
Thanks for doing those changes. I'm happy with everything so feel free to merge.
2 participants
Fixes #1114
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.