E3SM-Project / E3SM

Energy Exascale Earth System Model source code. NOTE: use "maint" branches for your work. Head of master is not validated.
https://docs.e3sm.org/E3SM
Other
354 stars 366 forks source link

Remove redundant declarations and commmented-out code in ocn_comp_mcf.F #6071

Open proteanplanet opened 1 year ago

proteanplanet commented 1 year ago

There are blocks of disused code in ocn_comp_mcf.F that have been left unattended for between four and eight years. I suggest it's time to remove these from the coupling code as part of a cleanup for V3, which will help make people's lives easier in the transition to Omega. Please look over https://github.com/E3SM-Project/E3SM/blob/proteanplanet/ocean/cleanup-ocncompmct/components/mpas-ocean/driver/ocn_comp_mct.F and compare it to master, and comment here as to whether you are happy with the removal of the commented-out code and the associated declarations that are unused. Please comment by this Friday, November 17, so that we can submit a BFB PR to master by next Monday, November 20, if all are in agreement for the removals.

vanroekel commented 1 year ago

I'm happy to proceed with the removals you suggest. Thanks @proteanplanet

rljacob commented 1 year ago

Looks ok to me. There is plenty of time to do code clean up because there will be a v3.1 and it is a few months away.

njeffery commented 1 year ago

@proteanplanet : I'd like to keep DMS flux. This code was commented out during the port of MarBL because we didn't have time to port DMS from BEC, not because we don't want it.

proteanplanet commented 1 year ago

Thanks @njeffery. Note that the DMS flux is wrong. It's not averaged over an ocean model coupling timestep, so it's better deleted than misused.

maltrud commented 1 year ago

@proteanplanet I disagree. it just needs to be corrected, not deleted. the DMSflux array was meant as a placeholder/reminder for when passing DMS became a priority. the other 2 arrays can be removed (CO2Flux, surfaceUpwardCO2Flux)

proteanplanet commented 1 year ago

@maltrud and @njeffery: I've added an altered commented-out code so that the DMS placeholder is still in there. I've also added a note that a registry entry would be required to make the code work. While I appreciate there is an intention to add DMS, nothing has been touched on this part of the code for eight years. Please let me know if this compromise is acceptable and what you were thinking.

njeffery commented 1 year ago

@proteanplanet: That works! Eight years is but a blink in the eye of bgc modeling...

maltrud commented 1 year ago

thanks @proteanplanet. please change the variable name to avgDMS_gas_flux for similarity with avgCO2_gas_flux. note also that it doesn't just need to be added to Registry to make it functional, there are changes to the coupler and mpas_ocn_time_average_coupled.F that need to be done, but probably not worth noting here.

rljacob commented 1 year ago

For everyone's info, here's what the ocean is currently sending to the coupler (from the coupler log in a v3b01 watercycle case)

seq_flds_o2x_states=
 So_t:So_s:So_u:So_v:So_dhdx:So_ssh:So_dhdy:So_bldepth:So_fswpen:So_blt:So_bls:So_htv:So_stv:So_rhoeff
seq_flds_o2x_fluxes= Faoo_h2otemp:Fioo_q:Fioo_frazil
proteanplanet commented 1 year ago

please change the variable name to avgDMS_gas_flux

@maltrud Your wish is my command. Change has been made.

We now need feedback on the freshwater ice commented-out code.

Also note that ssh is coupled from the state field, rather than a time-averaged field over the coupling timestep. That change will be made separately, and only affects WW3 and MOSART coupling in non-standard E3SM configurations.

maltrud commented 1 year ago

thanks, @proteanplanet.

maltrud commented 1 year ago

adding to what @rljacob noted about o2x coupler fields: for a G-case with ocean and ice BGC we have a lot more states sent from the ocean (and similarly for ice=>ocean i2x fields):

seq_flds_o2x_states= So_t:So_s:So_u:So_v:So_dhdx:So_ssh:So_dhdy:So_bldepth:So_fswpen:So_algae1:So_al gae2:So_algae3:So_doc1:So_doc2:So_doc3:So_dic1:So_don1:So_no3:So_sio3:So_nh4:So _dms:So_dmsp:So_docr:So_fep1:So_fep2:So_fed1:So_fed2:So_zaer1:So_zaer2:So_zaer3 :So_zaer4:So_zaer5:So_zaer6:So_blt:So_bls:So_htv:So_stv:So_rhoeff

with fully coupled carbon, the ocean also passes CO2 flux to the coupler for the atmosphere to take in: Faoo_fco2 when the time comes for prognostic DMS (currently EAM reads this in from a file), there will also be Faoo_fdms.

xylar commented 1 year ago

I would also support @rljacob's idea of putting this kind of clean-up on the back burner. I think we're all working pretty frantically to get essential functionality in by the deadline and just don't have the bandwidth for clean-up PRs until after the code freeze.

proteanplanet commented 1 year ago

The reason I have raised this now is that there have been multiple problems found in this file over the past month with real consequences for E3SM, now with two incorrectly coupled fields found. Cleaning up the code helps identify issues and is not just a matter of aesthetics. It would help me if we can have a clean working version. I am not yet finished going over coupling.