search

MetOffice / monio

Met Office NetCDF I/O (MONIO) written in C++ for file I/O in JEDI-based DA (JADA).
BSD 3-Clause "New" or "Revised" License
3 stars 2 forks source link

Feature/no field levels calls #32

Closed phlndrwd closed 10 months ago

phlndrwd commented 10 months ago

Further to the discussions here. It's unclear why Atlas offers the Field.levels() function as a call to a much slower metadata item. Particularly as it is prone to errors (for example if the metadata item isn't set and contradicts the size of the underlying data). However, Atlas does also offer the Field.shape() function. This item is carried out directly on the underlying data object (array_). See this line as compared to this line. Calls to Field.shape() are considered safer, and are presumably faster than calls to Field.levels().

For this reason, this PR replaces all calls to Field.levels() with calls to Field.shape(), and also replaces some uses of int with atlas::idx_t in applicable places.

Test outputs for MONIO, LFRic-Lite and LFRic-JEDI are here: http://fcm1/cylc-review/taskjobs/punderwo/?suite=all_no_levels_04

phlndrwd commented 10 months ago

Thanks @mo-joshuacolclough. Solid review! You suggestions were good! I didn't notice there was a Field.shape(i) method. It made sense to switch to that in applicable places. And the pass-by-value of dimensions to AtlasWriter::populateDataWithField() was probably left from some debugging code - its use goes back at least four months (I gave up looking for its origin beyond that). I've incorporated your suggestions and have re-tested: http://fcm1/cylc-review/taskjobs/punderwo/?suite=all_no_levels_05

mo-joshuacolclough commented 10 months ago

Thanks @phlndrwd :)

the pass-by-value of dimensions to AtlasWriter::populateDataWithField() was probably left from some debugging code

Ah ok, wasn't sure if it was intentional. I imagine it doesn't make much difference either way.

I've incorporated your suggestions

Did you push the changes? I can't see them....

phlndrwd commented 10 months ago

Thanks @phlndrwd :)

the pass-by-value of dimensions to AtlasWriter::populateDataWithField() was probably left from some debugging code

Ah ok, wasn't sure if it was intentional. I imagine it doesn't make much difference either way.

I've incorporated your suggestions

Did you push the changes? I can't see them....

No! I merged it before pushing. Had to create a second PR which I put in as an extension of this one: https://github.com/MetOffice/monio/pull/33