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

Accumulate data land-ice mass fluxes #5910

Merged

Conversation

xylar
Copy link
Contributor

@xylar xylar commented Sep 2, 2023

Before this merge, we were only accumulating this flux in prognostic flux mode. With this merge, we accumulate the mass flux in data mode as well, as this term is needed in the total mass budget of the model.

To facilitate this and reduce issues related to initialization vs. forward time stepping, this accumulation step has been broken into its own subroutine, ocn_surface_land_ice_fluxes_accumulate_fluxes(). This subroutine is called when time-stepping the model but not at initialization (since we do not want to accumulate any fluxes at that time).

Fixes #5911

Before this merge, we were only accumulating this flux in
prognostic flux mode.  With this merge, we accumulate the mass
flux in data mode as well, as this term is needed in the total
mass budget of the model.

To facilitate this and reduce issues related to initialization
vs. forward time stepping, this accumulation step has been broken
into its own subroutine, ocn_surface_land_ice_fluxes_accumulate_fluxes.
This subroutine is called when time-stepping the model but not
at initialization (since we do not want to accumulate any fluxes
at that time).
@xylar xylar added mpas-ocean BFB PR leaves answers BFB labels Sep 2, 2023
@xylar xylar requested review from darincomeau and removed request for cbegeman September 2, 2023 12:33
@xylar
Copy link
Contributor Author

xylar commented Sep 2, 2023

@jonbob, this is somewhere in the gray area between clean-up and a bug fix, but let's call it a bug fix.

I expect it to be bit-for-bit except for runs in DISMF mode, and only with those runs if the timeSeriesStatsGlobal are part of the checked output. Let me know if such at test exists, in which case we need to mark this as non-BFB.

Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

This falls into the category of additional reporting, and should definitely be bit-for-bit. I agree with the purpose, and reviewed by visual inspection. Thanks. I also did a quick compile test with gnu and intel on chrysalis using mpas-ocean stand-alone.

@xylar
Copy link
Contributor Author

xylar commented Sep 18, 2023

@darincomeau, can you take a look at this when you have time?

@rljacob
Copy link
Member

rljacob commented Sep 21, 2023

@darincomeau please review.

Copy link
Member

@darincomeau darincomeau left a comment

Choose a reason for hiding this comment

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

Approved by visual inspection - sorry for the delay!

jonbob added a commit that referenced this pull request Sep 25, 2023
…next (PR #5910)

Accumulate data land-ice mass fluxes

Before this merge, we were only accumulating this flux in prognostic
flux mode. With this merge, we accumulate the mass flux in data mode as
well, as this term is needed in the total mass budget of the model.

To facilitate this and reduce issues related to initialization vs.
forward time stepping, this accumulation step has been broken into its
own subroutine, ocn_surface_land_ice_fluxes_accumulate_fluxes(). This
subroutine is called when time-stepping the model but not at
initialization (since we do not want to accumulate any fluxes at that
time).

Fixes #5911
[BFB]
@jonbob
Copy link
Contributor

jonbob commented Sep 25, 2023

passes:

  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel
  • ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel

merged to next

@jonbob jonbob merged commit f2dc02e into E3SM-Project:master Sep 26, 2023
@jonbob
Copy link
Contributor

jonbob commented Sep 26, 2023

merged to master

@xylar
Copy link
Contributor Author

xylar commented Sep 26, 2023

@jonbob Thanks so much!

@xylar xylar deleted the mpas-ocean/accumulate-data-land-ice-fluxes branch September 26, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB bug fix PR mpas-ocean
Projects
None yet
Development

Successfully merging this pull request may close these issues.

accumulatedLandIceMass not being computed in DISMF runs
5 participants