search

marbl-ecosys / MARBL

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

Update settings_dict to hold more data #346

Closed mnlevy1981 closed 3 years ago

mnlevy1981 commented 4 years ago

rather than settings_dict[varname] being set to the value of the variable, it is now a dictionary with value, units, and longname keys. This is being done to make it easier to turn the YAML file into a table to publish either in a journal article or online through sphinx.

I don't think POP uses settings_dict directly, but this may require a small POP update as well.

mnlevy1981 commented 4 years ago

I tried using my branch in the latest version of POP and needed to modify MARBL_scripts/MARBL_wrappers/MARBL_settings.py; so POP will need some updates to use MARBL after we make this change.

I did want to mention that there is a little bit more data in settings file that we could consider bringing in to settings_dict:

   parm_o2_min :
      longname : Minimum O2 needed for production & consumption
      subcategory : 4. general parameters
      units : nmol/cm^3
      datatype : real
      default_value : 5.0

The subcategory is really just used for some internal bookkeeping in MARBL (and to make sure values are determined in the proper order in cases where one variable depends on another), but maybe I should include datatype as well? I could see that being useful information in the web documentation.

mnlevy1981 commented 3 years ago

@klindsay28 -- I'd like to merge this change in, but don't want to do so without a review. I think it might be small enough to look at asynchronously, but I'm happy to walk through it with you if you'd prefer (my calendar is up-to-date). The impetus for this change is that https://github.com/marbl-ecosys/cesm2-marbl/pull/37 pulls more than just the value of a parameter out of settings_dict... so before settings_dict[varname] returned the parameter value but now it's a dictionary containing the long name and units in addition to the value itself. This doesn't fix #341 but the PR in marbl-ecosys/cesm2-marbl will get us closer to addressing that issue.

Once this is approved and merged, I'll make a POP PR that adds ['value'] to all seven references to self._MARBL_settings.settings_dict in MARBL_scripts/MARBL_wrappers/MARBL_settings.py. I have a sandbox with all these changes made and have verified that all aux_pop_MARBL tests on cheyenne pass both NLCOMP and BASELINE, so marbl_in does not change relative to baselines and the history files are bit-for-bit.

klindsay28 commented 3 years ago

Would it be simpler for self._settings_dict_attrs to be a local variable in _update_settings_dict? It's not used outside of this function.

mnlevy1981 commented 3 years ago

Would it be simpler for self._settings_dict_attrs to be a local variable in _update_settings_dict? It's not used outside of this function.

yes it would! For some reason I thought there were a couple of routines that would need it, and when I noticed my changes were all in _update_settings_dict() I didn't think about moving it. Good catch!