marbl-ecosys / MARBL

Marine Biogeochemistry Library
https://marbl-ecosys.github.io
Other
14 stars 25 forks source link

Only allow GCM to request certain output fields if base_bio_on is True #453

Closed mnlevy1981 closed 9 months ago

mnlevy1981 commented 10 months ago

It doesn't make sense for MARBL to pass chlorophyll (surface or 3D), CO2 flux, O2 flux, or NHx flux if the base bio tracer module is inactive. Besides updating marbl_interface_public_types.F90 to check the value of base_bio_on, changes were made to the stand-alone driver so that the call_compute_subroutines test could still run with marbl_with_abio_only.settings.

mnlevy1981 commented 10 months ago

Two additional notes:

  1. c64e5b7 has expected failures because there are output fields in the call_compute_subroutines.history_with_abio_only.nc baseline that are no longer in the history file
  2. I also modified the pip requirements file for the documentation to pin alabaster; the latest version of that package is not compatible with the version of Sphinx we are using
mnlevy1981 commented 10 months ago

@klindsay28 and I had a conversation about this PR at lunch, and we both agree think it would be beneficial to have the initialization routines register what outputs can be provided (under what conditions; e.g. CO2 flux requires base_bio_on = True), and then marbl_single_output_constructor() can compare against what has been registered rather than using the clunky select case currently in the code.

This would make the output_to_GCM part of the code look more like the settings and diagnostics part of the code, though we definitely want to register all the possible outputs every run so that error messages can say "flux_co2" requires base_bio_on = True rather than "flux_co2" is not a valid output_for_GCM.

mnlevy1981 commented 10 months ago

Followup to the last comment -- introducing a registration for the output_for_GCM variables with an error_message argument could set up something like

if (base_bio_on) then
  error_message = ""
else
  error_message = "flux_co2 can only be provided when base_bio_on = True"
end if
call ???%register_output('flux_co2', error_message)

And then, when the GCM requests an output, we can loop through the registry. If len(trim(error_message)) == 0 then proceed with otherwise abort with error_message (and if an unregistered field is requested, abort with a generic {variable} not a valid output_to_GCM message)

mnlevy1981 commented 9 months ago

After chatting with @klindsay28 there are two tasks remaining

  1. Instead of returning some fields in marbl_instance%surface_flux_output and other fields via marbl_instance%get_output_for_GCM(), use a unified marbl_instance%output_for_GCM that can allocate memory for either a 2D or 3D field depending on what is requested. Basically, rename surface_flux_output to output_for_GCM and then make sure we can use the type to return full depth chlorophyll in addition to 2D fields like O2 flux, CO2 flux, etc
  2. Add a page to the documentation. Note that there are already a few pages that mention surface_flux_output and surface fluxes in the GCM. The first link will need to be updated to reflect the new setup, and the second link could use more detail under "2. Surface forcing fields, which may be requested by the GCM". The new documentation page for adding an output could link to one of those two pages under the heading "GCM Code Changes."
mnlevy1981 commented 9 months ago

In d572db6 there's a typo in an error message, so requesting flux_o2 when it is not available references flux_co2 rather than flux_o2 (the error message is Can not add flux_co2 to outputs without base biotic tracers and lflux_gas_o2); I'll fix this when I add a documentation page, otherwise the first task from https://github.com/marbl-ecosys/MARBL/pull/453#issuecomment-1939342964 has been addressed.

mnlevy1981 commented 9 months ago

Reviewed with @klindsay28 -- addressed most of the small concerns in 89cf5b9 but two big tasks remain:

mnlevy1981 commented 9 months ago

I don't know why it isn't showing up here, but 1c59f37 passed CI tests on my fork