Add CX and varobs for Surfacecloud

[PJLevensMO]

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]

@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]

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]

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]

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]

@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]

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.

[ctgh]

Thanks for adding this - it looks very good!
I have a couple of questions.

[ctgh]

Should the name Cloud match up with the name used in JCSDA-internal/ioda#1062?

[PJLevensMO]

Not necessarily, but maybe.

The way it works in OPS is that Cloud or cloud is the field that goes into the varobs, i.e. this one, it is the cloud fraction on model levels calculated in the SurfaceCloud routine, whereas the CX field is what is called level_cloud and I think is the one Fabien has added there. I'm not 100% sure what their interaction should be in JOPA, whether they should have the same name or not.

[james-cotton]

I think this needs to match with the name proposed in https://github.com/JCSDA-internal/ioda/pull/1062, i.e. cloudAmount instead of Cloud.

However I am happy if you want to commit this as is, and then handle the variable name change with separate PR to ioda, ufo, opsinputs, etc?

[fabien-mo]

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 JCSDA-internal/ufo-data#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.

[james-cotton]

Thanks Fabien, looks good

[james-cotton]

I don't think we should be writing out from DerivedObsError here (as mentioned in https://github.com/JCSDA-internal/ufo/pull/2957). Also can I check why there is a trailing underscore after cloudAmount?

[fabien-mo]

OK, thanks James.
Are you suggesting to replace DerivedObsError with EffectiveError, or simply remove it completely?
The underscore were @ctgh's suggestion to ensure that when the data is on model level, we get cloudAmount_1 to _70 and not cloudAmount1.

[james-cotton]

Ah I see, that makes sense regarding the underscores, thanks for clarifying.

We would want to replace DerivedObsError with ObsErrorData I think

[fabien-mo]

@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.

[james-cotton]

@fabien-mo thanks for the explanation. In that case we can proceed with this change.

[ctgh]

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