Added design document for OMEGA metadata management #13
Conversation
There was a problem hiding this comment.
@philipwjones Sorry for taking so long to review this! I left a few comments and I'm happy to chat more about fill value masking if you have questions.
| set of required metadata and interfaces must enforce this minimum set. | ||
| This minimum will be defined later, but for variables, this would | ||
| typically include a name (short), units, long name/description, | ||
| standard CF name (if exists), and dimensions. |
There was a problem hiding this comment.
Noting the need to specify invalid/"fill" values, e.g. for vertical levels below the seafloor.
There was a problem hiding this comment.
Added a fillValue as a required metadata entry
| // Dimensions - only for defining fields/variables | ||
| I4 nDims; // number of dimensions | ||
| std::vector<MetadataDim*> dims; // pointer to the defined | ||
| // dimension for each dim |
There was a problem hiding this comment.
In MPAS-Ocean to support fill-value masking we had to add a pointer mask to cellMask or edgeMask (and should include other masks such as landIceMask). I don't know if you had another way around this in mind or if we need something similar here.
There was a problem hiding this comment.
At this level, the fillValue is only specifying the value to be used as a fill so that apps can check for that value in the field. The actual filling and masking will take place elsewhere.
| const std::vector<MetadataDims*> // vector of dim pointers | ||
| ); | ||
|
|
||
| Do we want to require any other metadata (valid min/max, fill value)? |
There was a problem hiding this comment.
See above comments about fill value masking.
I think that including valid min/max metadata could be useful, that is, they could be used directly in the new version of ocn_validate_state to check whether values are in bounds.
There was a problem hiding this comment.
I think we need not only a fill value but a mask field (as @cbegeman mentioned above) for where that fill value should be applied (e.g. on output for streams that allow masking). It doesn't seem obvious that we would want to apply that fill value in-code because it might hamper efficiency. If we want masking to be applied to data written to restart files, we would need a way to know what the mask values should be in-code and on output so that they could be replaced with the in-code values when they are read in.
This may be too much for this design doc but wanted to make sure the relevant pieces make it in.
There was a problem hiding this comment.
add valid min/max as required metadata. see above comment about fill value and masking
| const std::vector<MetadataDims*> // vector of dim pointers | ||
| ); | ||
|
|
||
| Do we want to require any other metadata (valid min/max, fill value)? |
There was a problem hiding this comment.
I think we need not only a fill value but a mask field (as @cbegeman mentioned above) for where that fill value should be applied (e.g. on output for streams that allow masking). It doesn't seem obvious that we would want to apply that fill value in-code because it might hamper efficiency. If we want masking to be applied to data written to restart files, we would need a way to know what the mask values should be in-code and on output so that they could be replaced with the in-code values when they are read in.
This may be too much for this design doc but wanted to make sure the relevant pieces make it in.
- modified document format due to recent template change - added a group feature to create shorthand for groups of fields - changed many implementation details
7e646fd to
241c00f
Compare
|
@xylar and @cbegeman Can you please take another look at this? I've added a group feature to create a shorthand for groups of fields so that the contents in config files don't get too long (sorta like MPAS packages but at the metadata level). I've significantly changed a lot of the implementation details - my previous implementation would not have worked and ChatGPT had some good advice when I asked it for code ;^). But the functionality and requirements are largely the same - I did eliminate the model config-in-metadata and we'll do provenance in another way. Finally, I changed formatting to comply with the new template and documentation structure. |
cee/15.0.0 with GPU MPI buffers can crash in a system lib like this: #4 0x00007fffe159e35b in (anonymous namespace)::do_free_with_callback(void*, void (*)(void*)) [clone .constprop.0] () from /opt/cray/pe/cce/15.0.0/cce/x86_64/lib/libtcmalloc_minimal.so.1 #5 0x00007fffe15a8f16 in tc_free () from /opt/cray/pe/cce/15.0.0/cce/x86_64/lib/libtcmalloc_minimal.so.1 #6 0x00007fffe99c2bcd in _dlerror_run () from /lib64/libdl.so.2 #7 0x00007fffe99c2481 in dlopen@@GLIBC_2.2.5 () from /lib64/libdl.so.2 #8 0x00007fffea7bce42 in _ad_cray_lock_init () from /opt/cray/pe/lib64/libmpi_cray.so.12 #9 0x00007fffed7eb37a in call_init.part () from /lib64/ld-linux-x86-64.so.2 #10 0x00007fffed7eb496 in _dl_init () from /lib64/ld-linux-x86-64.so.2 #11 0x00007fffed7dc58a in _dl_start_user () from /lib64/ld-linux-x86-64.so.2 #12 0x0000000000000001 in ?? () #13 0x00007fffffff42e7 in ?? () #14 0x0000000000000000 in ?? () Work around this by using cee/14.0.3.
|
@philipwjones, sorry for the delay on this. I'll take a look as soon as I can, hopefully later today or tomorrow. |
There was a problem hiding this comment.
@philipwjones It seems like you've given this a lot of thought and I don't have additional feedback to offer at this point. The metadata groups seem appropriate to me. Thanks!
There was a problem hiding this comment.
@philipwjones, this looks excellent. I reread the document and it all looks great to me. Thank you for reformatting based on the updated template.
I'm very excited to work with this code.
Adds a design document for managing field and global metadata within OMEGA.