Skip to content

Add min error of 0.1 for airnow - #1792

Closed
huiying-noaa wants to merge 3 commits into
JCSDA-internal:developfrom
huiying-noaa:airnow_addminerr
Closed

Add min error of 0.1 for airnow#1792
huiying-noaa wants to merge 3 commits into
JCSDA-internal:developfrom
huiying-noaa:airnow_addminerr

Conversation

@huiying-noaa

@huiying-noaa huiying-noaa commented Jun 22, 2026

Copy link
Copy Markdown

Add min error of 0.1 for AirNOW. Please let me know if there's any comments.

Issues: current version has 0 error in our development test for PM2.5 DA for RRFS-SD https://github.com/NOAA-EMC/rrfs-workflow/tree/rrfs-mpas-jedi. The RDASApp used for RRFS DA directly points to this repo.

Simple PR, no dependency.

I have performed a self-review of my own code and tested results.
Original PM2.5 error: _, 0.05, 0.47, 0, 0.12
New PM2.5 error: _, 0.1, 0.47, 0.1, 0.12.

@huiying-noaa huiying-noaa added COMPO Atmospheric COMPOsition needs review Asking others to review - often used for pull requests labels Jun 22, 2026
@huiying-noaa
huiying-noaa requested review from EricJames-NOAA, guoqing-noaa, hongli-wang and weiwilliam and removed request for guoqing-noaa June 22, 2026 21:32

@EricJames-NOAA EricJames-NOAA left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me! Thank you for opening this PR.

@guoqing-noaa

Copy link
Copy Markdown

@huiying-noaa We will need to make sure the CI tests pass first.

@huiying-noaa

Copy link
Copy Markdown
Author

Hi @weiwilliam, Do you have any leads on the ci test error? Many thanks!

Node 20 is being deprecated. This workflow is running with Node 24 by default. If you need to temporarily use Node 20, you can set the ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION=true environment variable. For more information see: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
Run actions/create-github-app-token@v1
/home/runner/work/_actions/actions/create-github-app-token/v1/dist/main.cjs:42559
throw new Error("Input required and not supplied: private-key");
^

Error: Input required and not supplied: private-key
at Object. (/home/runner/work/_actions/actions/create-github-app-token/v1/dist/main.cjs:42559:9)
at Module._compile (node:internal/modules/cjs/loader:1854:14)
at Object..js (node:internal/modules/cjs/loader:1985:10)
at Module.load (node:internal/modules/cjs/loader:1577:32)
at Module._load (node:internal/modules/cjs/loader:1379:12)
at wrapModuleLoad (node:internal/modules/cjs/loader:255:19)
at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:154:5)
at node:internal/main/run_main_module:33:47

Node.js v24.16.0

@eap

eap commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

The problem with CI in this PR is because the PR is coming from a fork which does not have access to our org secrets. I just talked to Steve H who noted that this repo is expected to have future pull requests from collaborators without JCSDA write access. Therefore I will adopt the pull_request_target approach currently used by CRTMv3 which allows for testing of forks by running a checked-in version of the workflow.

I've created the PR #1794 to address the issue. Once that PR is merged, you can sync this and the tests should run as expected.

eap added a commit that referenced this pull request Jun 30, 2026
…ow. (#1794)

Change the CI trigger to use `on: pull_request_target:...` instead of "on: pull_request:..." this means that all pull requests, including pull requests from forks, will run CI using the checked-in config from the target branch.

This change has been made to enable fork pull requests to test. The previous strategy only allowed tests from branches of this repository forks.

__Issues:__

This change unblocks #1792
@weiwilliam

weiwilliam commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@huiying-noaa You need to create new testoutput. You can check

ecbuild_add_test( TARGET test_${PROJECT_NAME}_airnow
TYPE SCRIPT
ENVIRONMENT "PYTHONPATH=${IODACONV_PYTHONPATH}"
COMMAND bash
ARGS ${CMAKE_BINARY_DIR}/bin/iodaconv_comp.sh
netcdf
"${Python3_EXECUTABLE} ${CMAKE_BINARY_DIR}/bin/airnow2ioda_nc.py
-i testinput/airnow_2020081306.dat
-s testinput/airnow_sites.dat
-o testrun/airnow_2020081306.nc"
airnow_2020081306.nc ${IODA_CONV_COMP_TOL})
ecbuild_add_test( TARGET test_${PROJECT_NAME}_airnow_epalist
TYPE SCRIPT
ENVIRONMENT "PYTHONPATH=${IODACONV_PYTHONPATH}"
COMMAND bash
ARGS ${CMAKE_BINARY_DIR}/bin/iodaconv_comp.sh
netcdf
"${Python3_EXECUTABLE} ${CMAKE_BINARY_DIR}/bin/airnow2ioda_nc.py
-i testinput/airnow_2020081306.dat
-s testinput/airnow_sites_epalist_20240716.csv
--epa_list
-o testrun/airnow_2020081306_epalist.nc"
airnow_2020081306_epalist.nc ${IODA_CONV_COMP_TOL})
for reference.
After you created the new testoutputs, replace the test/testoutput/airnow_2020081306.nc and test/testoutput/airnow_2020081306_epalist.nc with them.
Then push another commit to your branch.
Thanks!

@huiying-noaa

Copy link
Copy Markdown
Author

Thanks for checking and providing the instructions, @weiwilliam!

@BenjaminRuston

Copy link
Copy Markdown
Collaborator

@huiying-noaa and note you can specify error dynamically, assume you are doing this, but adding this to a yaml inserts a value which doesn't necessarily even need to be in the input file:

  # Assign the initial observation error (constant value, 3.5 m/s right now).
  - filter: Perform Action
    filter variables:
    - name: windEastward
    - name: windNorthward
    action:
      name: assign error
      error parameter: 3.5

@mer-a-o mer-a-o left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assigns 0.1 for all species. Is it possible to make it for only PM2.5? This converter is used for other species such as O3 and NO2 and 0.1 may not be a suitable for these species. This requires some more changes to the converter but can address the issue properly.

@huiying-noaa

Copy link
Copy Markdown
Author

Thanks for your comments, @BenjaminRuston and @mer-a-o! I'll first try adding minimum error in yaml to achieve the same.

@BenjaminRuston

Copy link
Copy Markdown
Collaborator

@huiying-noaa do not believe you've got the species key capability added as yaml parameter correct?

@huiying-noaa

huiying-noaa commented Jul 24, 2026

Copy link
Copy Markdown
Author

Hi @BenjaminRuston , I'm mostly on vacation and sick leave after the last discussion. For the few tests I was able to conduct, the conditional error assignment in yaml by filtering with ObsValue or ObsError of pm2.5 hasn't been achieved yet. I do see the error assign process in log, but values all remain the same. I'll do more tests early next week and report back.

@huiying-noaa

Copy link
Copy Markdown
Author

Hello all, Many thanks for all your input. I was able to achieve the same results using yaml settings. All communication is helpful for my future commits. Thanks again!
Closing this PR now.

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.

8 participants