search

JCSDA-internal / ioda-converters

Various converters for getting obs data in and out of IODA
8 stars 2 forks source link

fixed the precision of TropOMI CO pressureVertice #1463

Closed weiwilliam closed 5 months ago

weiwilliam commented 5 months ago

Description

This PR fixes the precision of pressureVertice for TropOMI CO data.

Issue(s) addressed

Resolves #1462

Dependencies

List the other PRs that this PR is dependent on:

Impact

Expected impact on downstream repositories:

Checklist

weiwilliam commented 5 months ago

I just committed the change for you, that should pass the test now

Thank you, @jeromebarre !

PatNichols commented 5 months ago

@weiwilliam You need to fix the coding norms failure. The CI doesn't show the failure unless you look deeper

PatNichols commented 5 months ago

@weiwilliam The output file is not following the IODA conventions ( we are trying to clean these up at present). @jeromebarre @gthompsnJCSDA Could comment more about this.

jeromebarre commented 5 months ago

@weiwilliam The output file is not following the IODA conventions ( we are trying to clean these up at present). @jeromebarre @gthompsnJCSDA Could comment more about this.

The ctests have all passed. For the conventions we will do this in another set of PR. We don't have resources for this in this quarter. This PR (Bugfix) is good to go.

weiwilliam commented 5 months ago

Got it, I see the error message is 

  Variable MetaData/quality_assurance_value
   ERROR: Variable 'MetaData/quality_assurance_value' is not listed in the YAML conventions file.
jeromebarre commented 5 months ago

Got it, I see the error message is

  Variable MetaData/quality_assurance_value
   ERROR: Variable 'MetaData/quality_assurance_value' is not listed in the YAML conventions file.

This has been a while like this I think..... This is not the purpose of this PR which is again a bugfix for float type issue. We can work on this convention issues later.

jeromebarre commented 5 months ago

This file needs to pass ioda_validate.x, It's somewhat bad that our current ctests do not catch these failures.

This should be in another PR, this PR is just a float32 type bugfix. Thanks

PatNichols commented 5 months ago

@weiwilliam The output file is not following the IODA conventions ( we are trying to clean these up at present). @jeromebarre @gthompsnJCSDA Could comment more about this.

The ctests have all passed. For the conventions we will do this in another set of PR. We don't have resources for this in this quarter. This PR (Bugfix) is good to go.

This is embarrassing but the ctests using ioda_validate.x have been doing absolutely nothing for quite awhile. When someone adds a file we have to manually go look at the ctest output to see if it fails. There are currently ~100 files we will need to fix in a sprint so we want to keep from adding more to the pile. I know it sounds unfair but here we are. I will talk with the others and see what they think but I cannot promise anything.

PatNichols commented 5 months ago

@weiwilliam @jeromebarre Here is the output of ioda_validate.x on the nc4 file Reading YAML from /build_container/ioda/share/test/testinput/validation/ObsSpace.yaml Processing data file: /jcsda/ioda-bundle/iodaconv/test/testoutput/tropomi_co_total.nc Verifying that all required groups exist Verifying group MetaData Verifying group PreQC Verifying group ObsValue Verifying group RetrievalAncillaryData Verifying group ObsError Verifying group / Warning: Attribute 'Vertice' is present but is not listed as a required or optional attribute in the spec. Warning: Attribute 'Location' is present but is not listed as a required or optional attribute in the spec. Warning: Attribute 'nvars' is present but is not listed as a required or optional attribute in the spec. Warning: Attribute 'Layer' is present but is not listed as a required or optional attribute in the spec. Warning: Attribute 'date_time_string' is present but is not listed as a required or optional attribute in the spec. Warning: Attribute 'Vertice' does not have a YAML spec. Warning: Attribute 'Location' does not have a YAML spec. Warning: Attribute 'Layer' does not have a YAML spec. Verifying dimension names Verifying variable information Variable MetaData/dateTime Variable MetaData/latitude Variable MetaData/longitude Variable MetaData/quality_assurance_value ERROR: Variable 'MetaData/quality_assurance_value' is not listed in the YAML conventions file. Variable PreQC/carbonmonoxideTotal ERROR: Variable 'PreQC/carbonmonoxideTotal' is not listed in the YAML conventions file. Variable ObsValue/carbonmonoxideTotal ERROR: Variable 'ObsValue/carbonmonoxideTotal' is not listed in the YAML conventions file. Variable RetrievalAncillaryData/averagingKernel Variable RetrievalAncillaryData/pressureVertice Variable ObsError/carbonmonoxideTotal ERROR: Variable 'ObsError/carbonmonoxideTotal' is not listed in the YAML conventions file. Final results:

errors: 4

warnings: 8

srherbener commented 5 months ago

As I recall, the ioda_validate.x application takes command line options that tell it whether to ignore warnings and/or errors. If ioda_validate.x has the options that tell it to ignore warnigns and errors, then you get the reporting but a zero is returned and the test passes no matter what. If ioda_validate.x is told to not ignore warnings for example, if warnings do occur then a non-zero return code is issued (and the test should fail). Warnings and errors are treated as separate categories, so you can for example ignore warnings and not ignore errors.

I suspect the test may be using the option settings that cause ioda_validate.x to ignore both and always return zero.

weiwilliam commented 5 months ago

@srherbener Yes, you are correct, both arguments on. https://github.com/JCSDA-internal/ioda-converters/blob/0f7b1a74bd44b8f339deacf20f59b0a24a5be8ba/test/CMakeLists.txt#L2283-L2298

PatNichols commented 5 months ago

As I recall, the ioda_validate.x application takes command line options that tell it whether to ignore warnings and/or errors. If ioda_validate.x has the options that tell it to ignore warnigns and errors, then you get the reporting but a zero is returned and the test passes no matter what. If ioda_validate.x is told to not ignore warnings for example, if warnings do occur then a non-zero return code is issued (and the test should fail). Warnings and errors are treated as separate categories, so you can for example ignore warnings and not ignore errors.

I suspect the test may be using the option settings that cause ioda_validate.x to ignore both and always return zero.

@srherbener This happend during the Data Conventions sprint. There are about 100 files in ioda-converters and a few hundread in ufo-data that needed to be changed to pass. It was thought to be too much of a task at the time and hence we went into technical debt. Recently we noticed quite a bit of new files that were wrong and now we are trying to keep the problem from getting worse until we can fix it all in a sprint and turn off the --ignore-errors at least.

jeromebarre commented 5 months ago

While I understand the concerns, I will repeat myself until people read me here... conventions is not the point of this PR. I have created this issue to make sure we track this https://github.com/JCSDA-internal/ioda-converters/issues/1464 a lot needs to be updated across repos for this to be consistent. We don't have the resources for this in this quarter.