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

Local w3emc module, removing sp and bacio dependency #468

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scrasmussen
Copy link
Member

@scrasmussen scrasmussen commented May 2, 2024

The external w3emc dependency is added as a local module that is built by the project. The w3emc has been modified to use generic interfaces to handle different input types (for example real32 real64) to functions with the same name. The appropriate module use statements have been added.

Removing sp and bacio dependencies, which also need these w3emc changes.

This requires the w3emc module from CCPP Physics PR#1070

@scrasmussen scrasmussen force-pushed the w3emc-module branch 8 times, most recently from d600027 to 98d1095 Compare May 22, 2024 22:27
@scrasmussen scrasmussen changed the title Local w3emc module Local w3emc module, removing sp and bacio dependency May 22, 2024
@scrasmussen
Copy link
Member Author

Adding commits that remove the sp and bacio dependencies. Removing these required the new w3emc module so I am bundling all of the changes together. New updates have also been made to the documentation.

@scrasmussen
Copy link
Member Author

I should note that during testing the w3emc and bacio changes seemed to produce results identical to the baseline, for the twpice SCM_RAP. When I removed sp there were differences but I wasn't able to track down why.

@grantfirl
Copy link
Collaborator

I think that there needs to be a PR into fv3atm and that the ccpp/physics PR should be targeted to ufs/dev to get reactions from that community before it goes into here.

@grantfirl
Copy link
Collaborator

I think that there needs to be a PR into fv3atm and that the ccpp/physics PR should be targeted to ufs/dev to get reactions from that community before it goes into here.

@dustinswales @mkavulich Do you agree? I think that we should try to do w3, bacio, and sp at the same time, and since FV3 is the only model that has real dependence on sp, we need to see if it is OK to move splat from sp into sfcsub.f. I worry about this going into NCAR/main first, even though it would be nice for the release.

@dustinswales
Copy link
Collaborator

@grantfirl I concur. I think we should have time to get this in before the release. No?

@mkavulich
Copy link
Collaborator

mkavulich commented May 30, 2024

In my opinion it is best to remove the w3emc and bacio dependencies now (since that requires no additional work, and we can easily get it done for the release) and start the discussion on sp to be resolved at a later date.
I agree that starting the fv3atm PR in the https://github.com/NOAA-EMC/fv3atm repository is the way to go if dycore changes are needed; it's usually a bad thing to duplicate library functionality in code like this, so I anticipate some pushback even if it may be justified in this particular case.

@mkavulich
Copy link
Collaborator

Didn't see @dustinswales's comment before I replied, I have no problem with trying it all at once, it just seems like one more thing that is going to push against the release deadline.

@grantfirl
Copy link
Collaborator

This thread is relevant: ufs-community/ccpp-physics#41 (review)

@dustinswales
Copy link
Collaborator

@mkavulich I think we could do this quickly, but maybe I'm too optimistic?
All we need are these changes and add splat from sp into sfcsub and we're set.

@grantfirl
Copy link
Collaborator

In my opinion it is best to remove the w3emc and bacio dependencies now (since that requires no additional work, and we can easily get it done for the release) and start the discussion on sp to be resolved at a later date. I agree that starting the fv3atm PR in the https://github.com/NOAA-EMC/fv3atm repository is the way to go if dycore changes are needed; it's usually a bad thing to duplicate library functionality in code like this, so I anticipate some pushback even if it may be justified in this particular case.

Dom and Arun were both on board with removing the SP dependency and another person was planning on doing so in a different way. I think that it is OK to have code duplication in this case (from the SP library) because it is old, static code that may be possible to remove down the line anyway when/if someone refactors sfcsub.F.

@dustinswales
Copy link
Collaborator

In my opinion it is best to remove the w3emc and bacio dependencies now (since that requires no additional work, and we can easily get it done for the release) and start the discussion on sp to be resolved at a later date. I agree that starting the fv3atm PR in the https://github.com/NOAA-EMC/fv3atm repository is the way to go if dycore changes are needed; it's usually a bad thing to duplicate library functionality in code like this, so I anticipate some pushback even if it may be justified in this particular case.

Dom and Arun were both on board with removing the SP dependency and another person was planning on doing so in a different way. I think that it is OK to have code duplication in this case (from the SP library) because it is old, static code that may be possible to remove down the line anyway when/if someone refactors sfcsub.F.

I forgot that we discussed this a while back. I say we do it.
@scrasmussen I can help with rebasing and testing in the UFS if you aren't familiar with that.

@mkavulich
Copy link
Collaborator

In that case I agree, if there is already buy-in from the fv3atm side then going all at once is probably the best course of action.

…hysics

Updating documentation

Removing w3emc dependency installation from CI

Removing w3emc module load

Removing bacio and sp depedency
@scrasmussen scrasmussen marked this pull request as draft July 24, 2024 21:02
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.

4 participants