Sprint: Updating two GOES and three marine converters for naming conventions#1121
Conversation
gthompsnJCSDA
requested review from
BenjaminRuston,
ericlingerfelt and
srherbener
|
@ericlingerfelt It seems you are the really best candidate to review the PR please. |
|
I will take look this morning. Thanks for doing this, @gthompsnJCSDA! |
|
I can review the GOES converter modifications, but I do not have the expertise to review the marine ones. |
|
I decided to put this PR into DRAFT mode while I work out a bunch more marine converters to use the proper dateTime variable. Stay tuned. |
|
@gthompsnJCSDA is this ready for merging... |
Not yet. I will check off the last couple items and pull it out of |
BenjaminRuston
left a comment
BenjaminRuston
left a comment
There was a problem hiding this comment.
this looks good, thanks for working through these, the change fromncf to hdf will have to ask you more about later
| from datetime import datetime, date, timedelta | ||
| import os | ||
| from pathlib import Path | ||
| from pyhdf.SD import SD, SDC |
There was a problem hiding this comment.
Why switch to pyhdf API? What problem is this solving?
There was a problem hiding this comment.
Because I had to. The input file is hdf4, not hdf5/netcdf, so I had to switch. This is basically coming from a develop branch change that someone made - so I just followed it thru into the sprint branch so it functions again.
There was a problem hiding this comment.
also found this strange was this something from the aerosol group? but if pyhdf is supported in the stack would potentially get this through and then have the aerosol group review and hopefully consolidate and minimize the number of libraries used
There was a problem hiding this comment.
@gthompsnJCSDA thanks for the explanation. pyhdf is supported/included in spack-stack. It would be nice in the future to get providers updated from hdf4 to netcdf/hdf5, but that is certainly outside the scope of this PR.
|
I think it makes sense to merge now so that the downstream work gets unblocked. I'll do the merge now. Thanks for your patience! |
6 participants
Description
This is a large PR to update all non-BUFR python converters to the develop branch and get them all to work with the naming convention changes. In all there are 11 marine, 2 land, 5 composition converters along with the GOES converter and GNSSRO. Only the GOES converter does not have a corresponding ctest due to file sizes of the input files.
This PR does not resolve getting all ctests to pass for the ioda-validate program of the testoutput files. That will require additional changes to the source code again to update any variable names in correspondence with the
ObsSpace.yamlsince a number of new converters in marine and compo were added since the develop branch had gone forward for over 7 months without corresponding changes in the sprint branch.Issue(s) addressed
Part of the code sprint and an issue was not separately created.
Acceptance Criteria (Definition of Done)
Working in sprint branch, positive reviews, and output files pass validation tests (this step will come soon in next PR).
Dependencies
None
Impact
There should be none as long as kept within Sprint branch now.