search

MetOffice / opsinputs

JEDI library generating VarObs and Cx files
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

Add CX and varobs for Surfacecloud #175

Closed PJLevensMO closed 1 year ago

PJLevensMO commented 1 year ago

Adds namelists for SurfaceCloud for CX and varobs, as well as ctests. The varobs writer has needed to be updated to pass the correct output from JOPA to the cloud varfield. I have added VarField 15 and CX field 15, which are both needed for SurfaceCloud and were missing before.

fabien-mo commented 1 year ago

@ctgh Peter has submitted his opsinput PR. I've looked at the output of the CI tests but can't see anything that my tests/print statements were not already showing. Did I missed anything?

ctgh commented 1 year ago

Thanks for adding this. There are some useful clues around line 2236 of the CI output: Variable 15 requested but not present for SurfaceCloud No valid VarFields requested for obsgroup 42 I'm not entirely sure what's going on here but I would recommend adding some debug output to the varobswriter code.

fabien-mo commented 1 year ago

Thanks for adding this. There are some useful clues around line 2236 of the CI output: Variable 15 requested but not present for SurfaceCloud No valid VarFields requested for obsgroup 42 I'm not entirely sure what's going on here but I would recommend adding some debug output to the varobswriter code.

Yes, that's precisely what I've been investigating. I've track the problem down to deps/ops/code/OpsMod_ObsInfo/Ops_ObsGlobalAction.inc where I think the sub subroutine in Ops_ObsAction does not pick up the data correctly, header and El2 remain empty.

ctgh commented 1 year ago

There are a couple of things it's probably worth taking a look at: (1) In your yaml, the simulated variables section contains [dummy]. I think this should probably be [Cloud], and that there should be an derived variables section that contains the same. See other *_Varfields_*.yaml files for examples.

(2) In the netCDF file there are variables called things like DerivedObsValue/Cloud/lev1 (i.e. a nested group). I don't think that's right - Cloud should be the name of the variable (perhaps with channels).

Regarding channels, I wonder if it will be easier to debug this if you disregard them for now. In other words get it working assuming Cloud is a point variable to be assimilated. Once that works you could extend it to different levels. That's just an idea - given you have set up the machinery to use channels maybe you want to keep that and make the changes mentioned above. There are other varfields that use channels such as 052 that you could investigate.

fabien-mo commented 1 year ago

@ctgh, I've try to use the print option from Ops_ObAction, but this isn't printing anything, see:

https://github.com/MetOffice/opsinputs/blob/4e196ea595e3d0ad2e4d345d78b3dcad30e14b29/deps/ops/code/OpsMod_ObsInfo/Ops_ObsGlobalAction.inc#L1773C1-L1778C1

http://fcm1/cylc-review/view/fcarmina?&suite=PL_sith&no_fuzzy_time=0&path=log/job/20210701T1200Z/ukv_jopa_process_background_surfacecloud/01/job.out

fabien-mo commented 1 year ago

There are a couple of things it's probably worth taking a look at: (1) In your yaml, the simulated variables section contains [dummy]. I think this should probably be [Cloud], and that there should be an derived variables section that contains the same. See other *_Varfields_*.yaml files for examples.

(2) In the netCDF file there are variables called things like DerivedObsValue/Cloud/lev1 (i.e. a nested group). I don't think that's right - Cloud should be the name of the variable (perhaps with channels).

Regarding channels, I wonder if it will be easier to debug this if you disregard them for now. In other words get it working assuming Cloud is a point variable to be assimilated. Once that works you could extend it to different levels. That's just an idea - given you have set up the machinery to use channels maybe you want to keep that and make the changes mentioned above. There are other varfields that use channels such as 052 that you could investigate.

Hi Chris, sorry I've not clarified that earlier, I'm not trying to fix the Ctests, I'm trying to get it to work in a real sith run. I haven't looked at Peter's ctests at all, although, yes, arguably in the end it should be similar.

fabien-mo commented 1 year ago

Further to the naming convention discussed in https://github.com/JCSDA-internal/ioda/pull/1062, I've updated this PR so that Cloud and CloudError are now using cloudAmount.

I've also created a UFO https://github.com/JCSDA-internal/ufo/pull/2957 and UFO-data https://github.com/JCSDA-internal/ufo-data/pull/347 PR in line with this change.

Ctests are all OK and a test run in SITH yield matching results compare to before the naming change.

fabien-mo commented 1 year ago

@james-cotton I've discussed the possible implication re. JADA with @mikecooke77. This is not an issue to use a 'dummy' state variable in simulated variables and derived variables. Also, it is not necessary that cloudAmount be declared in any other these. For future JADA processing, there will be two options, either call/read in cloudAmount in JADA's yaml, a forward operator will then convert it to RH before the information can be assimilated, or the conversion to RH could be done directly in JOPA's yaml, saving the data in a derived variable hypothetically named surfacecloudRH or similar, which would be call/read in in JADA's yaml but the forward operator would simply be Identity. Should we opt for the second solution, the implementation of the conversion in JOPA will be a day 2 job.

Regarding the ctest varobswriter_ukvnamelist_surfacecloud.yaml, it is however more convenient to keep both a dummy state variable, here cloud_layer, and cloudAmount (both have the same dimension) in simulated and derived variable to ensure the test reads properly the data file.

ctgh commented 1 year ago

Thanks everyone. I'll merge this later today unless there are any further comments.