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

Bringing in shared constants #179

Open
xylar opened this issue Dec 4, 2024 · 28 comments
Open

Bringing in shared constants #179

xylar opened this issue Dec 4, 2024 · 28 comments
Labels
enhancement New feature or request

Comments

@xylar
Copy link

xylar commented Dec 4, 2024

@hyungyukang raised the issue:
#170 (comment)

should we start thinking about creating a shared constants file ?

I feel strongly that these need to come from:
https://github.com/E3SM-Project/E3SM/blob/09b9b7e21e3d381eafa962e178a5951580e2fc74/share/util/shr_const_mod.F90
or that we need to come up with a way to check that our constants have not deviated from these if they are separate.

I asked how Scream handling this, and @mwarusz said:

Not sure that they do:

https://github.com/E3SM-Project/E3SM/blob/09b9b7e21e3d381eafa962e178a5951580e2fc74/components/eamxx/src/physics/share/physics_constants.hpp#L46

https://github.com/E3SM-Project/E3SM/blob/09b9b7e21e3d381eafa962e178a5951580e2fc74/share/util/shr_const_mod.F90#L16

@xylar
Copy link
Author

xylar commented Dec 4, 2024

I'm sure not big on Omega and Scream keeping track of our own sets of constants. MPAS-Ocean's constants are different from the shared values in several annoying ways. Let's try very hard to avoid this!

@xylar xylar added the enhancement New feature or request label Dec 4, 2024
@mwarusz
Copy link
Member

mwarusz commented Dec 4, 2024

I was wrong, the pi value in Scream and shr_const_mod matches. It is just that shr_const_mod specifies more digits that can be represented. After looking more at these two files, I haven't found a constant that doesn't match. We still need to decide if we want to have our own file with the same values, or somehow get constants from one of these two files.

@xylar
Copy link
Author

xylar commented Dec 4, 2024

I think a separate location is problematic. I have that myself in MPAS-Tools:
https://github.com/MPAS-Dev/MPAS-Tools/blob/master/conda_package/mpas_tools/cime/constants.py
I at least check it in CI to make sure the constant match:
https://github.com/MPAS-Dev/MPAS-Tools/blob/master/conda_package/mpas_tools/tests/test_cime_constants.py

@xylar
Copy link
Author

xylar commented Dec 4, 2024

That's not good. Those kinds of inconsistencies are hard to fix later (e.g. when we have just done some of that in MPAS-Ocean, it required ~100 year simulations to evaluate the consequences) and they have real, physical consequences on the behavior of the coupled model.

@vanroekel
Copy link

I strongly agree with @xylar, having a separate constant file for omega seems a very bad idea to me.

@rljacob
Copy link
Member

rljacob commented Dec 5, 2024

cc @bartgol @AaronDonahue

@rljacob
Copy link
Member

rljacob commented Dec 5, 2024

I would suggest that we keep the shared physical constants in a text file which any program could ingest to get the shared values. shr_const_mod.F90 would be modifed to use that.

@bartgol
Copy link

bartgol commented Dec 5, 2024

I would suggest that we keep the shared physical constants in a text file which any program could ingest to get the shared values. shr_const_mod.F90 would be modifed to use that.

That seems reasonable. Now let's fight on the format :) Since EAMxx is in C++, I would vote for yaml. XML is also a reasonable choice, but since we already interface with yaml, that would save us from adding other dependencies and interface with other readers.

Tbc, EAMxx has also rolled its own constants file, but I am more than happy to just load in an existing file, so that all components agree on the value of constants. I am also fine with the driver exposing some functions along the line of

function get_physical_constant (name)  return (c)

(possibly with both a C and Fortran interface, or maybe making this function C-interoperable) leaving the burden to read the text file just to the driver...and possibly to the components trying to run in standalone mode.

@bartgol
Copy link

bartgol commented Dec 5, 2024

I found this repo in the ESCOMP org on github. Perhaps E3SM-Project could have its own or fork it (adding the constants we use/care about), with a few different formats. E.g., a namelist (for fortran), yaml/XML/json for C/Python. It seems like a reasonable thing for our org to have...

@xylar
Copy link
Author

xylar commented Dec 5, 2024

@bartgol, I would definitely be in favor of having a small external repo that houses a YAML file with the shared constants like in the ESCOMP link you shared. I don't feel strongly about whether there should be a parsing tool and I don't think a way to convert to a namelist would be better than just parsing the YAML file in shr_const_mod.F90.

The big advantage of the shared external repo is that it could easily be a submodule of E3SM and could also be used for other related repos like MPAS-Tools.

@bartgol
Copy link

bartgol commented Dec 5, 2024

My only reason for having a namelist file was to avoid adding another dep for a fortran parser for yaml (which I don't think we currently have). That said, I have never liked namelists, so I'd be pumped to see our F90 interact directly with yaml files instead.

I could start a repo with a constants yaml file, and a small README. Maybe we should quickly discuss what format do we want for that. The ESCOMP dictionary contains a lot of attributes for the constants:

         - name: stefan_boltzmann_constant
            value: 5.67036713E-08
            units: W m-2 K-4
            prec: double
            type: strict
            relative_uncertainty: 2.3E-06
            description: "
              Proportionality constant between the power emitted by
              a black body and the fourth power of its temperature, 
              according to Stefan-Boltzmann's law.
            "

I can't find the meaning of "type" in their docs, but anyways, I think name, value, units, and description are probably enough for our needs. I do like how they grouped them in sets though (math constants, fundamental constants, water constants, etc).

@rljacob
Copy link
Member

rljacob commented Dec 5, 2024

I would like us to just use the Physics Constants Dictionary. Although it currently lives in an NCAR repo, it was created as part of ICAMS and is meant to be used by all U.S. weather/climate models. Although we may be the first to actually do it.

@bartgol
Copy link

bartgol commented Dec 5, 2024

I would like us to just use the Physics Constants Dictionary. Although it currently lives in an NCAR repo, it was created as part of ICAMS and is meant to be used by all U.S. weather/climate models. Although we may be the first to actually do it.

Looking at that dictionary, it doesn't have many constants. I'm pretty sure we may want to add more. Maybe we could fork it?

Edit: but I'd still vote in favor of having an E3SM specific repo. Besides certain cryptic entries (what is type: strict?) or the fact that we don't really care about the "relative uncertainty", I don't really like the name of the sub-groups, as they are not self-explanatory for people not already familiar with them. E.g. IAPWS1995, SORCETIM2008, CODATA2014,... I would much prefer to see sections named mathematical, fundamental_physical, geophysical, atmospheric, etc.

@AaronDonahue
Copy link

Could someone post a link to the Physics Constants Dictionary? The ICAMS link takes me to their homepage but I couldn't find the dictionary.

@bartgol
Copy link

bartgol commented Dec 5, 2024

I found this repo in the ESCOMP org on github. Perhaps E3SM-Project could have its own or fork it (adding the constants we use/care about), with a few different formats. E.g., a namelist (for fortran), yaml/XML/json for C/Python. It seems like a reasonable thing for our org to have...

This link

@rljacob
Copy link
Member

rljacob commented Dec 5, 2024

Full disclosure: I'm on the ICAMS sub-committee that created the physics constants dictionary but was not involved in its creation. I can ask the developer about decisions like grouping constants by reference source instead of physical domain.

@rljacob
Copy link
Member

rljacob commented Dec 5, 2024

The reason for grouping them by source: PCD seeks to promote internal consistency and traceability of physical constants. So they're organized in to sets that come from the same findable reference.

@bartgol
Copy link

bartgol commented Dec 5, 2024

I'm not a huge fan of that approach. Most constants are not really up for debate (you don't need a reference for the value of pi, or speed of light or k_b...). I actually see this dictionary to be tailored for non-debatable constants. Stuff that is up for debate is probably a modeling choice, and should not be in such dictionaries... And even if you want to put "debatable" constants in the dictionary, you can add reference: <doi of paper introducing the constant> as an extra field for these particular constants...

@trey-ornl
Copy link

For whatever implementation you choose, I recommend that you have the requirement that the constants are defined at compile time so that they can be used in compile-time computations to minimize the number of runtime operations, maybe even predetermine some conditionals. One implementation would be to use a single .xml/.yaml/.json file to generate a Fortran parameter module and a C++ constexpr header at build time.

@xylar
Copy link
Author

xylar commented Dec 12, 2024

Like @bartgol, I found the existing list of constants from https://github.com/ESCOMP/PhysicalConstantsDictionary to be pretty limited. It seems like it would be a lot of politics and negotiation of we want to go down that route. I feel like we need a solution on a shorter timeline than that implies. We could come up with a solution that is similar to theirs and as compatible as possible but within our own repo for the time being.

I do very much like the idea of a repo dedicated to constants that could be an E3SM submodule but could also be a little conda package that could be a dependency of other tools like MPAS-Tools.

And I agree with @trey-ornl that having a way to convert the yaml file (or whatever) to code at build time would be efficient and would also avoid the need to have a yaml parser for Fortran or C++ for this purpose.

@xylar
Copy link
Author

xylar commented Dec 12, 2024

@bartgol, is this something you would have the bandwidth to get started?

@bartgol
Copy link

bartgol commented Dec 12, 2024

I could do some steps, like

  • create the repo, with a README
  • create an initial yaml file, with a bunch of fundamental constants, grouped by category (if that's something we like (I do)).
  • cook up a small script that generates a C++ header or a F90 module from the yaml one (I'm guessing a python script may be the most portable across e3sm projects?)

Testing, and expanding to other target languages can be done in a second phase.

@rljacob
Copy link
Member

rljacob commented Dec 12, 2024

I would like us to just use the Physics Constants Dictionary and at least its yaml syntax: https://github.com/ESCOMP/PhysicalConstantsDictionary/wiki/Dictionary-Syntax . It also already has the yaml -> Fortran piece done. Its been forked here: https://github.com/E3SM-Project/PhysicalConstantsDictionary

@bartgol
Copy link

bartgol commented Dec 12, 2024

Rob, I saw you comment now, and had already put together a quick and dirty attempt this morning, after replying to Xylar. I'd like us to have a simple py script for yaml->f90 and yaml->C++ conversion, since that's very contained and widely understandable (seeing autoconf stuff in the ESCOMP repo brings me back 20 years). And the yaml formatting is also not very understandable: do we need to group our constants using a paper as a group name? I don't think certain constants need a paper (pi is pi, and k_b is k_b). It's good that we have a fork, so we can change a few things, but I wonder how much you are comfortable with e3sm-project drifting apart, since a very large drift defies the fork approach...

@xylar
Copy link
Author

xylar commented Dec 13, 2024

I tend to agree, @bartgol, we are likely to drift pretty far from PhysicalConstantsDictionary on our fork by necessity because of model-specific needs and I don't know how useful that will end up being. But we can at least give it a try as @rljacob has requested. We can always move the (useful) contents of our fork to a different repo later to sever the connection if it isn't meaningful.

@rljacob
Copy link
Member

rljacob commented Dec 17, 2024

The point of PhysicalConstantsDictionary is meant for multiple modeling groups to share constant values. Truly model-specific constants would continue to be in things like shr_const_mod.

We may want to move this to https://github.com/E3SM-Project/PhysicalConstantsDictionary/discussions

@pbosler
Copy link

pbosler commented Dec 17, 2024

Late to join, but this came up a while ago when we started working on aerosols for scream: E3SM-Project/EKAT#48. I made a .yaml database with the constants I could find by grepping through e3sm's various components. It's a different format than pcd (because I hadn't talked to @rljacob yet) but it is aligned with e3sm.

I'll make an issue to continue the discussion in the new fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants