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

replace 3d array with 2d array in add_psfc_increment #59

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

RussTreadon-NOAA
Copy link
Contributor

Follow on to #57 with 2d array replacing 3d array in add_psfc_increment (file src/netcdf_io/calc_analysis.fd/inc2anl.f90)

Resolves #56

@RussTreadon-NOAA
Copy link
Contributor Author

@CatherineThomas-NOAA , when you have time would you check to see if the changes in this PR to src/netcdf_io/calc_analysis.fd/inc2anl.f90 allow the C1152 case to successfully run to completion.

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-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, thanks @RussTreadon-NOAA don't worry I won't click merge until we have confirmation from @CatherineThomas-NOAA that everything works

@CatherineThomas-NOAA
Copy link
Contributor

Thanks @RussTreadon-NOAA! The code looks good to me. I'll try it at C1152, though I will likely need to wait until WCOSS comes back later this week. We've been having issues on Hercules lately.

@RussTreadon-NOAA
Copy link
Contributor Author

@CatherineThomas-NOAA . Thanks for the update. Let's hold this PR until we confirm that it works for C1152.

@CatherineThomas-NOAA
Copy link
Contributor

I can confirm that this fix allows the C1152 case to run.

I'm thinking more about this code in general though. I believe that this code change is equivalent to what was already there, but I'm not sure if what was there is what we should be doing?

In the GSI minimization, we compute a increment for ps and then project it through the column:
delp(:,:,k) = pssm * (bk5(k)-bk5(k+1)) * r1000

In this utility, it is only pulling the increment from the lowest level delp to represent the surface. But the surface pressure should be impacted by increments to all levels above it. Do we instead need something like...

read 3D delp_inc
do loop k
ps_inc = ps_inc + delp_inc(k) / (bk5(k) - bk(k-1))
end do

@CoryMartin-NOAA
Copy link
Contributor

@CatherineThomas-NOAA

The equation in GSI you provided:
delp(:,:,k) = pssm * (bk5(k)-bk5(k+1)) * r1000
let's simplify it (remove the r1000)
delp(:,:,k) = pssm * (bk5(k)-bk5(k+1))
If we assume pssm is the surface pressure increment, then solving for it gets us:
pssm = delp(k) / (bk5(k)-bk5(k+1))

I think what the GSI code is doing is saying "the surface pressure increment is applied as a scaling factor to all layers based on their thickness", so thus we should be able to get that increment back, right?

@CatherineThomas-NOAA
Copy link
Contributor

@CoryMartin-NOAA: I agree, this process should be reversible.

Since a change like this would alter results and need more thorough investigation, how about we merge this PR as is so the C1152 g-w PR can advance, and then open a new issue to investigate the ps inc calculation? This PR would continue to replicate the behavior already used in our experiments.

@CoryMartin-NOAA
Copy link
Contributor

@CatherineThomas-NOAA works for me

@CoryMartin-NOAA CoryMartin-NOAA merged commit e6ce414 into develop Jan 3, 2025
4 of 8 checks passed
@CoryMartin-NOAA CoryMartin-NOAA deleted the bugfix/calcanal branch January 3, 2025 17:55
WalterKolczynski-NOAA pushed a commit to NOAA-EMC/global-workflow that referenced this pull request Jan 10, 2025
There are a few updates needed to cycle with the C1152 atmospheric
model:
- config.resources(.$machine): include C1152 case options for DA tasks
- gdasfcst config.ufs: WRTTASK_PER_GROUP_PER_THREAD_PER_TILE_GDAS=20 
- source code update for calc_analysis
NOAA-EMC/GSI-utils#59
- resource change needed for atmanl upp

Note that any needed changes to time limits for C1152 will be addressed
at a later date.

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

Calc_analysis read fix for C1152
3 participants