Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated test data #347

Closed
wants to merge 5 commits into from
Closed

Conversation

fabien-mo
Copy link

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

  • [x ] I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • [ x] I have run the unit tests before creating the PR

@james-cotton
Copy link
Contributor

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
Copy link
Author

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.

Copy link
Contributor

@PJLevensMO PJLevensMO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these changes Fab. I think the met_office_surfacecloud_cloud_base_height_check.nc4 file may need a further change for the changes James flagged in the ufo PR - this version still has DerivedObsError/CloudError where I think it should be DerivedObsError/cloudAmount. This may cause the ctests in the other PR to fail as it won't be testing the correct variable.

@fabien-mo
Copy link
Author

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

Copy link
Contributor

@PJLevensMO PJLevensMO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Fab. I think with the change James has just requested in the obs function in ufo this change becomes necessary. So it is good to have!

@james-cotton
Copy link
Contributor

james-cotton commented Aug 7, 2023

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

fabien-mo commented Aug 7, 2023

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
Copy link
Author

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

@james-cotton
Copy link
Contributor

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
Copy link
Author

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.

@fabien-mo fabien-mo closed this Aug 16, 2023
@fabien-mo fabien-mo deleted the feature/surfacecloud_fabien-mo branch August 16, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants