Moving all local constants to those in header file

#417 Merged at 7d8fe75
Repository
enzo-dev-constants
Branch
week-of-code
Repository
enzo-dev
Branch
week-of-code
Author
  1. Andrew Emerick
Reviewers
Description

This (large) PR converts all uses of locally defined and redundant physical constants in all .C and .F files to those as defined in either phys_constants.h or phys_const.def respectively.

This is not an exhaustive search to define only global constants. Constants that aren't defined in the header files already were not touched throughout the code, but in principle we can go back through and update this PR by including additional constants in the header file.

In cases where the header variable was used, but was renamed locally, the local rename was removed and the header variable was added in its place. In cases where constants were defined but never used, these were removed entirely.

This is done for now, but likely this should not be accepted until all soon-to-be-accepted PRs are accepted. There will be many merge conflicts with this, but I am happy to go through and deal with them.

Edit: In some cases the header variables had a different precision than the variables defined locally. In this case there will be answer changes and this will break the test suite. In cases were the locally defined constant had a higher precision, the header fine was updated to match this precision. This was not common.

  • Commit status

Comments (19)

    1. Andrew Emerick author

      Thanks for pining people to look at this…. and thank you to whoever decides to pour over this.

      But just as a heads up, this will likely generate some gross merge conflicts that would need to be resolved before accepting. My plan was to deal with those once all outstanding PRs that are likely to be accepted are accepted. But I’m happy to merge now to see what happens. Otherwise, let me know when everything else is all set to go and I’ll update this PR.

  1. Nathan Goldbaum

    This is definitely a worthy goal. That said the diff is huge and it’s going to take me a little while to go through it carefully, I’d prefer not to rubber-stamp 🙂 . Will try to do this next week sometime.

    1. John Wise

      Thanks! Take your time since we have to accept the new cosmology tests first since it will change the gold standard.

  2. Nathan Goldbaum

    Just a comment that I think it would be worth not leaving in any old constants and using the same constants everywhere, even if it causes some test answers to change.

  3. Nathan Goldbaum

    Looked over everything, only saw a few issues which I commented below. Thanks for pushing on this, this is a great cleanup.

  4. Brian O'Shea

    @aemerick , I looked through it as well and made some comments. In general, I think that you should take them as advisory; some suggestions were made about conventions before I appreciated that the fortran and C routines are defined somewhat differently (i.e., pi vs. pi_val, etc.). Overall, this looks great and I think it’s in really good shape. It will definitely slightly break a great many of the tests, though!

  5. Andrew Emerick author

    Thanks @ngoldbaum , @jwise77 , and @bwoshea .

    I’ll update this PR today with your suggestions and attempt to merge with the current tip.

  6. John Wise

    Now that the new cosmology test PR has been merged. We can now run the test suite on it and compare with the new gold standard. I will do this later in the week.

    1. Andrew Emerick author

      Thanks John! I’d expect this to break the gold standard, but interested to see how big a difference this makes.

  7. Britton Smith

    I think it’s time to get this thing merged. We just updated the gold-standard, which this will most likely break, but I think it would be a good idea just to see what all passes and fails. @aemerick, could you merge this with the latest tip and update the PR? That should run the test suite. Then I think we should do this.

    1. Britton Smith

      Actually, sorry, I see you’ve already done that, but it doesn’t look like the tests ran. You can manually run them by clicking on the Commits tab above, clicking on the latest commit, and then clicking on the link that says “run pipeline” in the upper right.

        1. Andrew Emerick author

          Just ran. and – to the surprise of no one – its failed. Do I need to do anything from here to update the gold standard? Or does someone else handle that?

          1. Britton Smith

            I’ve looked through the test output and, to my eye, saw only the type of failures I think we expect, i.e., differences of a few percent or so in all sorts of field value comparisons. In my opinion, I think it would be a good idea to update the gold standard as part of this PR. That way, we can see the tests run one more time and verify that nothing serious broke (like simulations crashing). I’ll defer to John and Greg on whether to do this and when.

          2. John Wise

            Thanks, Andrew. My thoughts are the same as Britton’s. Greg or I will update the gold standard after we merge your changes.

  8. John Wise

    I looked through all of failures and errors. The failures were caused by errors on the order of 1e-2, so I think this is fine. There were ~10 errors caused by PressurelessCollapse, which comes from several parameters being written as NaNs. This problem existed without these changes, and I will create an issue and fix it soon. After that PR is accepted (hopefully it won’t take long), I will create a new gold standard because I don’t want to create one without a PressurelessCollapse answer.

    I’m merging.