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

EAMxx: Create IOPForcing ATM process #6787

Merged

Conversation

tcclevenger
Copy link
Contributor

Moves IOP forcing from within HOMME interface to it's own physics process, called directly after dynamics process. Also, changes name of IntensiveObservationPeriod class to IOPDataManager.

I expect this to be non-BFB, at the very least since we are changing the order in which IOP forcing is applied to FM fields.

@tcclevenger tcclevenger self-assigned this Nov 29, 2024
@tcclevenger tcclevenger added non-BFB PR makes roundoff changes to answers. EAMxx PRs focused on capabilities for EAMxx labels Nov 29, 2024
@tcclevenger tcclevenger changed the title Create IOPForcing ATM process EAMxx: Create IOPForcing ATM process Dec 2, 2024
Copy link
Contributor

@bartgol bartgol left a comment

Choose a reason for hiding this comment

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

I have one comment regarding using the Field utility, but other than that, looks good.

Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

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

One thing comes to mind when looking at this PR ... is there a way we can extract more stuff out of this IOP process? For example, do we need to do nudging here or can we accommodate the IOP nudged cases in our already existing nudging process?

We can ofc defer/punt until later, just thinking out loud for code simplicity/readability/modularity

@bogensch
Copy link
Contributor

bogensch commented Dec 4, 2024

@mahf708 this is a good point and I think it's probably possible to eventually merge the IOP nudging routine to the EAMxx general one. However, I'd prefer to punt this to a different PR because I don't think it's trivial. For instance, the IOP nudging routine nudges the domain mean, not the individual column explicitly. Secondly, either the EAMxx nudging routine would need to be modified to support reading in the format of the IOP nudging OR the DPxx workflow and input files would need to be substantially revised. Just two things off the top of my head that come to mind.

@mahf708
Copy link
Contributor

mahf708 commented Dec 4, 2024

@mahf708 this is a good point and I think it's probably possible to eventually merge the IOP nudging routine to the EAMxx general one. However, I'd prefer to punt this to a different PR because I don't think it's trivial. For instance, the IOP nudging routine nudges the domain mean, not the individual column explicitly. Secondly, either the EAMxx nudging routine would need to be modified to support reading in the format of the IOP nudging OR the DPxx workflow and input files would need to be substantially revised. Just two things off the top of my head that come to mind.

My vote would be to edit the eamxx nudging routine so that it can accommodate the specific needs of iop as opposed to the other way around. But I agree, we could/should wait until later. No rush to do anything now

Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

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

Looks good!

Two minor comments (though no action is strictly needed):

  • I noticed a lot of barrier/fence usage that wasn't clear to me; just noting it here in case these were added for debugging only and left there accidentally
  • One thing that can really help is adding some sort of standalone unit testing. For example, you can see the unit tests in rrtmgp or spa or nudging. These can be helpful in learning the code, debugging the code, and also profiling the code. So, while this is still hot in your mind and if and only if you have time, I think adding tests can be quite nice

Copy link
Contributor

@bartgol bartgol left a comment

Choose a reason for hiding this comment

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

Looks good. Can you please ninja delete the IO cout line we discussed on slack, so it goes in with this?

@tcclevenger
Copy link
Contributor Author

@bogensch Can you test one more time before we merge? Particularly something that uses iop_nudging_uv/tq?

Moves IOP forcing from within HOMME interface to it's own physics
process, called directly after dynamics process. Also, changes name of
IntensiveObservationPeriod class to IOPDataManager.
if (iop_nudge_tq or iop_nudge_uv) {
  ...
  if (iop_nudge_tq or iop_nudge_uv) {
    ...
  }
}
@tcclevenger tcclevenger force-pushed the tcclevenger/eamxx/iop_as_atm branch from 9004f23 to efeca26 Compare December 9, 2024 13:42
@tcclevenger tcclevenger requested a review from bartgol December 9, 2024 13:42
@bogensch
Copy link
Contributor

bogensch commented Dec 9, 2024

@tcclevenger I ran a case that nudges U & V winds by default. The run crashed about six hours in. I ran the case with nudging turned off and the case ran fine.

For the case with nudging that crashed I noticed that the U and V being output were about four orders of magnitudes larger than expected.

@tcclevenger
Copy link
Contributor Author

@bogensch Thanks, definitely relate to my last round of changes. I'll investigate.

@tcclevenger tcclevenger force-pushed the tcclevenger/eamxx/iop_as_atm branch from efeca26 to 8b76d0f Compare December 9, 2024 20:55
@tcclevenger
Copy link
Contributor Author

@bogensch Were you testing on CPU? I think this was an issue with how I was using (or not using) Packs.

@bogensch
Copy link
Contributor

@tcclevenger yes, my testing was done on pm-cpu. I did not test on pm-gpu.

Fixes a bug where real views were being used as if they were packed.
@tcclevenger
Copy link
Contributor Author

@bogensch Can you test again?

@bogensch
Copy link
Contributor

@tcclevenger my tests on both pm-cpu and pm-gpu look good! Thus, this gets my seal of approval.

tcclevenger added a commit that referenced this pull request Dec 19, 2024
Moves IOP forcing from within HOMME interface to it's own physics process, called directly after dynamics process. Also, changes name of IntensiveObservationPeriod class to IOPDataManager.

[non-BFB]
@tcclevenger tcclevenger merged commit 6165581 into E3SM-Project:master Dec 20, 2024
17 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EAMxx PRs focused on capabilities for EAMxx non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants