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

NEMS driver cleanup #98

Merged

Conversation

DeniseWorthen
Copy link
Contributor

Fixes issue #95

  • The large number of line changes (>3000) in this PR is challenging. The commits were done stepwise:

c96dd29: remove NEMS mediator files
ac9558c: switch to yaml field dictionary, remove unused/legacy model fronts
8bbeb2a: remove ESMF<8 conditional blocks and code
ae01a66: clean up error logging
6f7ea44: simplify error messaging if required component is not found

DeniseWorthen and others added 30 commits October 4, 2019 10:57
merge noaa-emc/nems develop
Change acc# on WCOSS to GFS-DEV (#17)
debug mode for NEMS, additional minor changes in component mk files (…
Code changes to support DEBUG compilation based on appBuilder specifi…
remove ice-ocean merge in prep_atm (#24)
Update submodule pointer for NCEPLIBS-pyprodutil (NOAA-EMC#51)
Remove CCPP dynamic build from NEMS (NOAA-EMC#54)
support for ufs-s2s-model debug compilaiton (NOAA-EMC#40)
Bugfix i2a & Add Orion to module-setup (NOAA-EMC#61)
* update cice6 component mk

* update path for forapps/ufs

* update paths to MOM6 and CICE interfaces (#33)

Co-authored-by: Rahul Mahajan <aerorahul@users.noreply.github.com>

Co-authored-by: Rahul Mahajan <aerorahul@users.noreply.github.com>
* remove NEMS mediator files and Spaceweather Mediator file
* use fd_nems.yaml for FieldDictionary
* remove unused FRONT_COMPONENT and DriverAddComp:
* NMMB,GSM,MOM5,CICE5,POM,WRFHYDRO
* "S" components, "X" components
@DusanJovic-NOAA
Copy link
Collaborator

Consider removing module_NEMS_INTERNAL_STATE.F90. It does not contain any useful state, just two dummy variables.

@DusanJovic-NOAA
Copy link
Collaborator

Now that we removed ensemble coupler there's no need to have both NEMS and EARTH components, one should be enough. Basically 'nems' should be just a main program and one grid component.

@DusanJovic-NOAA
Copy link
Collaborator

I also do not see where two variables in EARTH_INTERNAL_STATE:

        real(ESMF_KIND_R8)  :: medAtmCouplingIntervalSec
        real(ESMF_KIND_R8)  :: medOcnCouplingIntervalSec

are used. If they are not needed anymore, maybe module_EARTH_INTERNAL_STATE.F90 can be removed as well.

@DeniseWorthen
Copy link
Contributor Author

DeniseWorthen commented Apr 19, 2021

There will be one more PR to clean up the driver itself but I think removing the NEMS internal state file would be OK at this point.

@DusanJovic-NOAA
Copy link
Collaborator

Five subroutines from module_NEMS_UTILS.F90 (err_msg, err_msg_int, err_msg_val, err_msg_var and err_msg_final) are also not used anywhere. CHECK_ESMF_PET can be moved to MAIN_NEMS.F90. Which basically means module_NEMS_UTILS.F90 can be removed.

Functions RTC() and TIMEF() can be removed from MAIN_NEMS.F90

@DeniseWorthen
Copy link
Contributor Author

I'm going to retain the NEMS_UTILS SR for now since I may want to move some items there in the second round.

I'm not sure what to do about the two INTERNAL_STATE SRs. I'm not confident enough of their purpose to actually remove them.

@DusanJovic-NOAA
Copy link
Collaborator

I'm going to retain the NEMS_UTILS SR for now since I may want to move some items there in the second round.

I'm not sure what to do about the two INTERNAL_STATE SRs. I'm not confident enough of their purpose to actually remove them.

None of the variables from two internal states are used. They (internal states) can be removed.

@junwang-noaa
Copy link
Collaborator

junwang-noaa commented Apr 20, 2021

I'd suggest to check with Mark Potts before removing the NEMS/earth internal state file. I am not sure if the JEDI-NEMS interface has been set up yet and if NEMS internal state will be used in that setting.

@DusanJovic-NOAA
Copy link
Collaborator

I'd suggest to check with Mark Potts before removing the NEMS/earth internal state file. I am not sure if the JEDI-NEMS interface has been set up yet and if NEMS internal state will be used in that setting.

I do not see how and why would JEDI access to nems and earth internal states. As the name suggests they are internal. If JEDI needs anything from nems grid component it should access it via import and/or export states.

@mark-a-potts Can you please check if JEDI needs access to NEMS internal state.

@mark-a-potts
Copy link
Contributor

I'd suggest to check with Mark Potts before removing the NEMS/earth internal state file. I am not sure if the JEDI-NEMS interface has been set up yet and if NEMS internal state will be used in that setting.

I do not see how and why would JEDI access to nems and earth internal states. As the name suggests they are internal. If JEDI needs anything from nems grid component it should access it via import and/or export states.

@mark-a-potts Can you please check if JEDI needs access to NEMS internal state.

The plan is for JEDI to get all information via NUOPC imports and exports, so it will not use the internal states.

@DeniseWorthen
Copy link
Contributor Author

The EARTH_INTERNAL_STATE does appear to be used in the module_EARTH_GRID_COMP (is%EARTH_INT_STATE)?

@DusanJovic-NOAA
Copy link
Collaborator

The EARTH_INTERNAL_STATE does appear to be used in the module_EARTH_GRID_COMP (is%EARTH_INT_STATE)?

EARTH_INTERNAL_STATE is declared, allocated, stored in the grid component and finally deallocated. But it's never actually used. Same for nems internal state. They can be removed.

@DusanJovic-NOAA
Copy link
Collaborator

Great. Now we just need to merge nems and earth component into one grid component, move check_esmf_pet to main program or use some alternative way of setting log type, figure out what to about these two rusage files and we'll be down to just main program and one grid component module. But that's for the next PR.

@MinsukJi-NOAA
Copy link
Collaborator

@DeniseWorthen is this pr ready for commit?

@DeniseWorthen
Copy link
Contributor Author

Yes it is but we haven't been merging the subcomponent PRs until the ufs-weather is updated w/ the RTs. Did you want to merge it now?

@MinsukJi-NOAA
Copy link
Collaborator

@DeniseWorthen Yes let's wait for the ufs-weather RT results.