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

fixed the precision of TropOMI CO pressureVertice #1463

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

weiwilliam
Copy link
Contributor

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

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

Copy link
Contributor

@jeromebarre jeromebarre left a comment

Choose a reason for hiding this comment

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

Thx!

@jeromebarre jeromebarre self-requested a review February 13, 2024 20:28
@@ -125,7 +125,7 @@ def _read(self):
preslv = ncd.groups['PRODUCT'].groups['SUPPORT_DATA'].\
groups['DETAILED_RESULTS'].variables['pressure_levels'][:]
preslv = np.reshape(preslv, (nlocs, nlevs))
top = np.zeros(nlocs)
top = np.zeros(nlocs,dtype=np.float32)
Copy link
Contributor

Choose a reason for hiding this comment

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

iodaconv_compo_coding_norms is failing. leave a space after the comma.

Copy link
Contributor

@jeromebarre jeromebarre left a comment

Choose a reason for hiding this comment

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

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

@weiwilliam
Copy link
Contributor Author

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

Thank you, @jeromebarre !

@PatNichols
Copy link
Contributor

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

@PatNichols
Copy link
Contributor

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

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

@PatNichols PatNichols added COMPO Atmospheric COMPOsition ALGO JEDI Algorithms labels Feb 14, 2024
@fcvdb fcvdb removed the COMPO Atmospheric COMPOsition label Feb 14, 2024
@PatNichols PatNichols removed the ALGO JEDI Algorithms label Feb 14, 2024
@fcvdb fcvdb added COMPO Atmospheric COMPOsition needs review Asking others to review - often used for pull requests labels Feb 14, 2024
Copy link
Contributor

@PatNichols PatNichols left a comment

Choose a reason for hiding this comment

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

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

@weiwilliam
Copy link
Contributor Author

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

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

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

PatNichols commented Feb 14, 2024

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

@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
Copy link
Collaborator

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

@srherbener Yes, you are correct, both arguments on.

foreach ( f ${TESTOUTPUT_IODA_FILES} )
get_filename_component(filename ${f} NAME)
# Note: IODA_YAML_ROOT is provided by find_package(ioda).
# The ObsSpace.yaml file is *not* a test file. It always exists.
ecbuild_add_test(
TARGET ${PROJECT_NAME}_validate_testoutput_${filename}
COMMAND ioda-validate.x
LABELS ${PROJECT_NAME}_validate
ENVIRONMENT "ECKIT_COLOUR_OUTPUT=1"
ARGS "--ignore-warn"
"--ignore-error"
"${IODA_YAML_ROOT}/validation/ObsSpace.yaml"
"${f}"
)
endforeach()

@PatNichols
Copy link
Contributor

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

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

@PatNichols PatNichols merged commit b283308 into develop Feb 15, 2024
6 checks passed
@PatNichols PatNichols deleted the bugfix/fix_tropomi_co branch February 15, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMPO Atmospheric COMPOsition needs review Asking others to review - often used for pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] pressureVertice of tropomi_co converted IODA file should be in single precision (float32)
6 participants