search

JCSDA-internal / ufo-data

Tier-1 test files for ufo repository
1 stars 0 forks source link

updated test data #347

Closed fabien-mo closed 1 year ago

fabien-mo commented 1 year ago

Description

This is an update following a variable name modification as discussed in https://github.com/JCSDA-internal/ioda/pull/1062. Cloud is now referred to as cloudAmount for surfacecloud obstype.

Dependencies

List the other PRs that this PR is dependent on:

Impact

No

Checklist

james-cotton commented 1 year ago

Hi @fabien-mo, can you clarify why there are ufo-data changes needed when cloudAmount is a derived obsvalue?

Apart from what look like rounding differences, the only change I can see in these 2 files are changes to what were missing data values for stationPressure

fabien-mo commented 1 year ago

That's correct, I've updated the missing data. Originally, filled with _, they were causing the ctests to fail. I've now added some dummy values, it does impact the value of the derived variables and the ctests are now working as expected.

fabien-mo commented 1 year ago

Thanks Peter, you're right, it's currently not needed but it good to be future proof. Now updated.

james-cotton commented 1 year ago

As this is just a variable renaming there shouldn't need to be any data changes? If the ctests are failing then that needs investigating as they were previously passing fine.

In met_office_surfacecloud_cloud_column_check you have DerivedObsError/CloudError DerivedObsValue/Cloud ObsValue/level_cloud

and in met_office_surfacecloud_cloud_base_height_check you have DerivedObsValue/Cloud

These are the varaibles that are renamed, however since the obsfunction is used to create the derived values/errors, can't they simply be removed from the input file?

fabien-mo commented 1 year ago

As this is just a variable renaming there shouldn't need to be any data changes? If the ctests are failing then that needs investigating as they were previously passing fine.

In met_office_surfacecloud_cloud_column_check you have DerivedObsError/CloudError DerivedObsValue/Cloud ObsValue/level_cloud

and in met_office_surfacecloud_cloud_base_height_check you have DerivedObsValue/Cloud

These are the varaibles that are renamed, however since the obsfunction is used to create the derived values, can't they simply be removed from the input file?

I've renamed them all, except level_cloud which is what you get in the output file (ODB name). Peter has generated these test file based on true JOPA file created in sith, that's why you've got everything, including many variables not used or needed by the current ctest. But Peter's mentioned above, they might be needed in future ctest. Since it's working, I'm not sure it's worth removing them.

I'll rerun the ctest on dev to check what's up with the missing data and report back here.

PJLevensMO commented 1 year ago

As this is just a variable renaming there shouldn't need to be any data changes? If the ctests are failing then that needs investigating as they were previously passing fine.

In met_office_surfacecloud_cloud_column_check you have DerivedObsError/CloudError DerivedObsValue/Cloud ObsValue/level_cloud

and in met_office_surfacecloud_cloud_base_height_check you have DerivedObsValue/Cloud

These are the varaibles that are renamed, however since the obsfunction is used to create the derived values, can't they simply be removed from the input file?

I think the references to Cloud and level_cloud could be removed, but I think the CloudError (now renamed) may need to be there still? I can't remember if the ObsFunction creates that variable or requires it to be present already in the obs space.

fabien-mo commented 1 year ago

OK, running the 2 ctests, test_ufo_function_surfacecloudmodellevelcbh and test_ufo_function_surfacecloudcreatecloudcolumn, using ufo and ufo_data dev results in the test passing, i.e., not failing, but all data are flagged as missing:

223: QC SurfaceCloud stationPressure: 25 missing values. 223: QC SurfaceCloud stationPressure: 0 passed out of 25 observations.

I don't think the change in opsinputs would cause that. Could it be that something changed in ufo between now and when Peter has put the tests in, which wasn't pickedup because the tests are not failing?

fabien-mo commented 1 year ago

Can someone also approve this companion of https://github.com/JCSDA-internal/ufo/pull/2957 please?

james-cotton commented 1 year ago

As this is just a variable renaming there shouldn't need to be any data changes? If the ctests are failing then that needs investigating as they were previously passing fine. In met_office_surfacecloud_cloud_column_check you have DerivedObsError/CloudError DerivedObsValue/Cloud ObsValue/level_cloud and in met_office_surfacecloud_cloud_base_height_check you have DerivedObsValue/Cloud These are the varaibles that are renamed, however since the obsfunction is used to create the derived values, can't they simply be removed from the input file?

I've renamed them all, except level_cloud which is what you get in the output file (ODB name). Peter has generated these test file based on true JOPA file created in sith, that's why you've got everything, including many variables not used or needed by the current ctest. But Peter's mentioned above, they might be needed in future ctest. Since it's working, I'm not sure it's worth removing them.

I'll rerun the ctest on dev to check what's up with the missing data and report back here.

@fabien-mo, did you get to the bottom of why you needed to change the missing data values in station pressure?

fabien-mo commented 1 year ago

All right @james-cotton, I've now re built the bundle with the fresh opsinputs changes and my branch of ufo, but using ufo-data from Dev. Now the test are passing OK and with the correct number of valid data. Not sure what was happening before. Anyway, this PR is no longer needed, I'm closing it.