NCAR / ccpp-scm

CCPP Single Column Model
Other
13 stars 50 forks source link

Local w3emc module, removing sp and bacio dependency #468

Open scrasmussen opened 2 months ago

scrasmussen commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

mkavulich commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

This thread is relevant: https://github.com/ufs-community/ccpp-physics/pull/41#pullrequestreview-1338053722

dustinswales commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.