Skip to content

Update dev/emc for JEDI (SOCA) + UFS#107

Closed
climbfuji wants to merge 38 commits intoNOAA-EMC:dev/emcfrom
climbfuji:feature/ufs-bundle-dom
Closed

Update dev/emc for JEDI (SOCA) + UFS#107
climbfuji wants to merge 38 commits intoNOAA-EMC:dev/emcfrom
climbfuji:feature/ufs-bundle-dom

Conversation

@climbfuji
Copy link

@climbfuji climbfuji commented Nov 23, 2022

Description

These updates are needed for running the JEDI Ocean DA component (soca) standalone or within the Unified Forecast System. For each of the data types that have been changed from private to public, I went back again and tested if they really need to be.

Note The PR is currently not up to date with develop. I will do that once I know whether the changes presented here are acceptable or not.

@JessicaMeixner-NOAA
Copy link
Collaborator

@climbfuji - Just wanted to check back in on this PR. The last comment you were going to check a few things and I don't know if you've had a chance or not.

@climbfuji
Copy link
Author

The whole merge back of our changes in the ufs-weather-model and its components is held up by the transition to fms@2023.02 at JCSDA and in the UFS. The new fms is now available in spack-stack, so there is a good chance that I'll be working on this in November.

@JessicaMeixner-NOAA
Copy link
Collaborator

@climbfuji thanks for that information. Does that mean that you do need the change from private -> public? If so, we might want to start the conversation with GFDL now to be prepared for next month. If you'd prefer to wait until the FMS issues are sorted out that works too.

@climbfuji
Copy link
Author

I really don't know, it's been such a long time, sorry. Let's wait until the fms update is resolved if that's ok?

@JessicaMeixner-NOAA
Copy link
Collaborator

@climbfuji that's okay with me.

@JessicaMeixner-NOAA
Copy link
Collaborator

@climbfuji I saw an update to this PR, so I thought I'd restart the conversation on the private-> public variables to see if those are still needed and if we needed to start a conversation with GFDL about these changes.

@climbfuji
Copy link
Author

@climbfuji I saw an update to this PR, so I thought I'd restart the conversation on the private-> public variables to see if those are still needed and if we needed to start a conversation with GFDL about these changes.

@JessicaMeixner-NOAA Thanks for the reminder, indeed timely! I am just about to finish an update of the ufs-weather-model and various JEDI components in the JEDI ufs-bundle. We should be able to discuss these changes next week if that work for you?

@JessicaMeixner-NOAA
Copy link
Collaborator

We have a tag-up with GFDL on Tuesday so if we know which variables will go from private->public before then I'll be sure to bring that up to see if there are concerns.

@climbfuji climbfuji force-pushed the feature/ufs-bundle-dom branch from 729f207 to a415a1b Compare December 15, 2023 16:31
!> Control structure for the MOM module, including the variables that describe
!! the state of the ocean.
type, public :: MOM_control_struct ; private
type, public :: MOM_control_struct ;
Copy link
Author

Choose a reason for hiding this comment

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

I checked that this must be public.

#include <MOM_memory.h>

public :: horiz_interp_and_extrap_tracer, myStats, homogenize_field
public :: horiz_interp_and_extrap_tracer, myStats, homogenize_field, meshgrid!, fill_miss_2d
Copy link
Author

Choose a reason for hiding this comment

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

both meshgrid and fill_miss_2d are needed by soca, too

!! with a format that is private so it can be readily changed without disrupting
!! other coupled components.
type, public :: ocean_state_type ; private
type, public :: ocean_state_type ;
Copy link
Author

Choose a reason for hiding this comment

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

This is also needed, because:

/Users/heinzell/work/ufs-bundle/ufs-bundle-update-from-develop-20231212/ufs-weather-model/MOM6-interface/MOM6/config_src/drivers/nuopc_cap/mom_cap_methods.F90:533:65:

  533 |           isc, iec, jsc, jec, ocean_grid%ke, ocean_state%MOM_CSp%T, ocean_grid, rc=rc)
      |                                                                 1
Error: Component 'mom_csp' at (1) is a PRIVATE component of 'ocean_state_type'
/Users/heinzell/work/ufs-bundle/ufs-bundle-update-from-develop-20231212/ufs-weather-model/MOM6-interface/MOM6/config_src/drivers/nuopc_cap/mom_cap_methods.F90:543:65:

  543 |           isc, iec, jsc, jec, ocean_grid%ke, ocean_state%MOM_CSp%S, ocean_grid, rc=rc)
      |                                                                 1
Error: Component 'mom_csp' at (1) is a PRIVATE component of 'ocean_state_type'
/Users/heinzell/work/ufs-bundle/ufs-bundle-update-from-develop-20231212/ufs-weather-model/MOM6-interface/MOM6/config_src/drivers/nuopc_cap/mom_cap_methods.F90:553:65:

  553 |           isc, iec, jsc, jec, ocean_grid%ke, ocean_state%MOM_CSp%u, ocean_grid, rc=rc)
      |                                                                 1
Error: Component 'mom_csp' at (1) is a PRIVATE component of 'ocean_state_type'
/Users/heinzell/work/ufs-bundle/ufs-bundle-update-from-develop-20231212/ufs-weather-model/MOM6-interface/MOM6/config_src/drivers/nuopc_cap/mom_cap_methods.F90:563:65:

  563 |           isc, iec, jsc, jec, ocean_grid%ke, ocean_state%MOM_CSp%v, ocean_grid, rc=rc)
      |                                                                 1
Error: Component 'mom_csp' at (1) is a PRIVATE component of 'ocean_state_type'

@climbfuji climbfuji force-pushed the feature/ufs-bundle-dom branch from 826bb13 to a720257 Compare December 16, 2023 02:55
!! with a format that is private so it can be readily changed without disrupting
!! other coupled components.
type, public :: ocean_state_type ; private
type, public :: ocean_state_type ;
Copy link
Author

Choose a reason for hiding this comment

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

This needs to be public

#include <MOM_memory.h>

public :: horiz_interp_and_extrap_tracer, myStats, homogenize_field
public :: horiz_interp_and_extrap_tracer, myStats, homogenize_field, meshgrid, fill_miss_2d
Copy link
Author

Choose a reason for hiding this comment

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

Both meshgrid and fill_miss_2d need to be public, too

@climbfuji climbfuji changed the title DO NOT MERGE YET - Update dev/emc for JEDI+UFS Update dev/emc for JEDI+UFS Dec 16, 2023
@climbfuji climbfuji changed the title Update dev/emc for JEDI+UFS Update dev/emc for JEDI (SOCA) + UFS Dec 16, 2023
@climbfuji climbfuji marked this pull request as ready for review December 16, 2023 02:59
@climbfuji
Copy link
Author

@JessicaMeixner-NOAA I went back and checked for each of the variables that were made public if it's still required. I left comments in the PR (self-review). I also updated the PR description and the title. Please let me know about the feedback you get on Tuesday. If the changes are acceptable, I can update the branch to the head of dev/emc and then it's ready to merge from our side. Thanks!

@JessicaMeixner-NOAA
Copy link
Collaborator

Thanks @climbfuji - I will let you know what feedback we get.

@JessicaMeixner-NOAA
Copy link
Collaborator

@climbfuji as suspected, there is a lot of concern from GFLD about making these variables private. The preference is to keep these private and create methods to access the variables that are needed. Tagging @jiandewang to see if he wants to add to this here.

@climbfuji
Copy link
Author

@climbfuji as suspected, there is a lot of concern from GFLD about making these variables private. The preference is to keep these private and create methods to access the variables that are needed. Tagging @jiandewang to see if he wants to add to this here.

Sigh. More delays for the JEDI-UFS integration then. Well, as long as GFDL creates those methods in a reasonable timeframe, it should be ok. What do you/they need for that? A list of the exact components of the data structures (in cases where we had to make the entire derived type public)?

@JessicaMeixner-NOAA
Copy link
Collaborator

@climbfuji I don't think there's any expectation that GFDL is going to create these methods for us. We've mentioned that this is likely an issue for a while now, so this isn't really a surprise here.

@climbfuji
Copy link
Author

@JessicaMeixner-NOAA Good news (for now). Because of PR JCSDA/ufs-bundle#43 we don't need this PR anymore. Our workaround for now is to apply a patch when we build the ufs-weather-model as an external project in the JEDI ufs-bundle. It achieves the same as what this PR was intended to do. We may come back in a year or so to revisit the issue, but for now we're good.

@climbfuji climbfuji closed this Feb 1, 2024
@JessicaMeixner-NOAA
Copy link
Collaborator

I believe @guillaumevernieres is working on something so that we don't have to have a patch or other things. @guillaumevernieres can you chime in on this and the work-around here?

@guillaumevernieres
Copy link
Collaborator

I believe @guillaumevernieres is working on something so that we don't have to have a patch or other things. @guillaumevernieres can you chime in on this and the work-around here?

I'm not working on anything related to this yet, but that's the plan @JessicaMeixner-NOAA .

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.

6 participants