E3SM icon indicating copy to clipboard operation
E3SM copied to clipboard

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

Open proteanplanet opened this issue 8 months ago • 14 comments

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.

proteanplanet avatar Nov 16 '23 03:11 proteanplanet

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

vanroekel avatar Nov 16 '23 04:11 vanroekel

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.

rljacob avatar Nov 16 '23 05:11 rljacob

@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.

njeffery avatar Nov 16 '23 16:11 njeffery

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.

proteanplanet avatar Nov 16 '23 17:11 proteanplanet

@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)

maltrud avatar Nov 16 '23 17:11 maltrud

@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.

proteanplanet avatar Nov 16 '23 20:11 proteanplanet

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

njeffery avatar Nov 16 '23 20:11 njeffery

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.

maltrud avatar Nov 16 '23 20:11 maltrud

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

rljacob avatar Nov 16 '23 22:11 rljacob

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.

proteanplanet avatar Nov 16 '23 23:11 proteanplanet

thanks, @proteanplanet.

maltrud avatar Nov 16 '23 23:11 maltrud

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.

maltrud avatar Nov 16 '23 23:11 maltrud

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.

xylar avatar Nov 17 '23 01:11 xylar

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.

proteanplanet avatar Nov 17 '23 02:11 proteanplanet