Skip to content

Handle bias correction files for aerosol DA #3694

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

Merged
merged 27 commits into from
Jun 23, 2025

Conversation

ypwang19
Copy link
Contributor

@ypwang19 ypwang19 commented May 14, 2025

Description

This PR is to stage and tar bias correction files for aerosol DA in the workflow.
This PR depends on #3711. (The file is staged and passed aerosol cycling DA CI test)

Resolves #3172
Refs (NOAA-EMC/GDASApp#1690)

Type of change

  • New feature (adds functionality)

Change characteristics

  • Is this a breaking change (a change in existing functionality)? NO
  • Does this change require a documentation update? NO
  • Does this change require an update to any of the following submodules? YES (If YES, please add a link to any PRs that are pending.)
    • GDAS

How has this been tested?

  • Cycled test on Hera

Checklist

  • Any dependent changes have been merged and published
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have documented my code, including function, input, and output descriptions
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • This change is covered by an existing CI test or a new one has been added
  • Any new scripts have been added to the .github/CODEOWNERS file with owners
  • I have made corresponding changes to the system documentation if necessary

CoryMartin-NOAA pushed a commit to NOAA-EMC/GDASApp that referenced this pull request May 14, 2025
# Description

This PR updates the root path of aerosol obs bias correction to match
that in global-workflow.

# Companion PRs
NOAA-EMC/global-workflow#3694

# Issues
Refs NOAA-EMC/global-workflow#3172

# Automated CI tests to run in Global Workflow
- [ ] C96C48_hybatmaerosnowDA  <!-- JEDI aero/snow cycled DA !-->

Co-authored-by: ypwang19 <yaping.wang@users.noreply.github.com>
DavidNew-NOAA
DavidNew-NOAA previously approved these changes May 14, 2025
aerorahul
aerorahul previously approved these changes May 20, 2025
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

lgtm

@aerorahul
Copy link
Contributor

The PR description says there is an update required to GDASapp submodule, but I don't see it in the list of changes.
I want to confirm that the description is accurate and we are waiting for an updated gdasapp hash in this PR.

@CoryMartin-NOAA
Copy link
Contributor

Most importantly we need the bias correction dummy files staged

@aerorahul
Copy link
Contributor

Ok. I will revert this to draft then.

@aerorahul aerorahul marked this pull request as draft May 21, 2025 12:43
@CoryMartin-NOAA
Copy link
Contributor

@aerorahul for future reference, can you provide criteria for what constitutes a draft PR vs an open PR? My impression (which perhaps is not the same as yours) is that draft means the changes herein are not ready for review, not that it is waiting on something like a hash update or staging a fix file.

@aerorahul
Copy link
Contributor

I was about to merge this PR, because the changes were approved. I did not realize that this PR needs other changes to be made or staged or communicated. None of that is communicated in this PR or its issue. At the very least, a link to the issue that is blocking this PR should be made. Barring all of this, makes this PR not ready -- hence a draft.

@aerorahul
Copy link
Contributor

I do agree, GitHub's PR state is bi-modal (Ready or Draft). I suppose our need is to have a mention of review this PR, and its companion PR(s).

@CoryMartin-NOAA
Copy link
Contributor

image
The linked issue is here in the PR. I'll admit that the title could be clearer but it is here and @KateFriedman-NOAA has been communicating with @ypwang19 on what is needed to get these staged.

@CoryMartin-NOAA
Copy link
Contributor

@aerorahul I would suggest some sort of label policy like "needs fix file" or "ready for CI" or "ready to merge"

@aerorahul
Copy link
Contributor

One can mention anything in the long list of comments, it is extremely difficult to sift out meaningful information from comments if the PR description is not clear. What this does, what its waiting on, etc.

@aerorahul aerorahul added blocked Issue is currently being blocked by another issue. Include blocking issue # in description needs fix file Needs a fix file to be staged (Include issue # in description) labels May 21, 2025
@aerorahul
Copy link
Contributor

@CoryMartin-NOAA
I created a label "needs fix file" and applied it here. I also added a label description to remind users to put the fix file issue number in the PR description.

@CoryMartin-NOAA
Copy link
Contributor

@aerorahul sounds good, I hope this will be helpful in the future

@aerorahul
Copy link
Contributor

Is there anything we can help with to move this PR along?

@CoryMartin-NOAA
Copy link
Contributor

@aerorahul @ypwang19 has been on leave. I think once she merges in my proposed fix, it will be ready for testing

@CoryMartin-NOAA
Copy link
Contributor

whoops, @ypwang19 please revert that PR. I made a mistake and kept pushing to that branch

@ypwang19
Copy link
Contributor Author

It should work now. Please test it. @aerorahul

Copy link
Contributor

@DavidHuber-NOAA DavidHuber-NOAA left a comment

Choose a reason for hiding this comment

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

Changes look good.

@DavidHuber-NOAA DavidHuber-NOAA added CI-Hercules-Ready **CM use only** PR is ready for CI testing on Hercules and removed CI-Hercules-Failed **Bot use only** CI testing on Hercules for this PR has failed labels Jun 18, 2025
@DavidHuber-NOAA DavidHuber-NOAA removed the request for review from WalterKolczynski-NOAA June 18, 2025 12:38
@emcbot emcbot added CI-Hercules-Building **Bot use only** CI testing is cloning/building on Hercules CI-Hercules-Running **Bot use only** CI testing on Hercules for this PR is in-progress CI-Hercules-Passed **Bot use only** CI testing on Hercules for this PR has completed successfully and removed CI-Hercules-Ready **CM use only** PR is ready for CI testing on Hercules CI-Hercules-Building **Bot use only** CI testing is cloning/building on Hercules CI-Hercules-Running **Bot use only** CI testing on Hercules for this PR is in-progress labels Jun 18, 2025
@DavidHuber-NOAA DavidHuber-NOAA merged commit aec6caf into NOAA-EMC:develop Jun 23, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-Hercules-Passed **Bot use only** CI testing on Hercules for this PR has completed successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle VarBC files for aerosol DA
7 participants