search

knelli2 / spectre

SpECTRE is a code for multi-scale, multi-physics problems in astrophysics and gravitational physics.
https://spectre-code.org/
Other
0 stars 1 forks source link

Remove fot option names #8

Closed knelli2 closed 3 years ago

knelli2 commented 3 years ago

Proposed changes

This does a lot of stuff. Maybe too much? But I don't know how to break it up into smaller bits... The first commit is self contained so that could be its own PR.

But the second commit is kinda big. It does two major things:

  1. Removes options for functions of time names from all possible classes/structs.
  2. Passes a std::unordered_map<string, double> representing a map between function of time names and initial expiration times to all the functions_of_time() methods of all DomainCreators and all TimeDependences. Then the TimeDependence checks if the name from the map matches its internal name, and if it does, uses the new expiration time instead of infinity. To do this a few other things were changed in addition (not major):
    • Control system OptionHolder was modified
    • domain::Tags::FunctionsOfTime was modified to loop over the OptionHolders for all present control systems, get their names and expiration times, then create and pass the std::unordered_map<string, double> to the domain creator (which will pass it to its time dependences if it has any).
    • Added a template parameter Index to the UniformTranslation which basically keeps track of how many UniformTranslation time dependences you have. The default behavior has not changed, i.e. UniformTranslation<2> will create a UniformTranslation in 2D with the name "Translation" and is constructable by options the usual way UniformTranslation:. However, if you need an additional UniformTranslation, it can be created with UniformTranslation<2, 1> and by options UniformTranslation1:

There are some questions/comments I have about this.

  1. When I check for OptionHolders for control systems, I assume there is a type alias control_systems in the metavars. Not sure how to enforce this/check this/require this/whatever.
  2. I need to add tests for a lot of these things. I edited existing tests so they all pass, but I haven't really written tests for these new features. I'll have to do that as well...
  3. Right now there's no check that names of the control systems are guaranteed to match the names of the functions of time in the TimeDependences/DomainCreators (wherever they are defined). Is this actually an issue? Right now we don't actually have any control systems soooooooo it's a non-issue?? If we did want to check somehow, what would that look like? How would we get info about the control system names to the TimeDependence/DomainCreator in a general way? Are we even going to want to control those TimeDependences? The control system is intended to be primarily for binary compact objects anyway.
  4. Is this all a terrible idea and I've been wasting my time?

Upgrade instructions

Code review checklist

Further comments

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1361612447

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Domain/Creators/DomainCreator.hpp 0 1 0.0%
src/Domain/Creators/TimeDependence/CompositionCubicScaleAndUniformRotationAboutZAxis.cpp 21 22 95.45%
src/Domain/Creators/BinaryCompactObject.cpp 13 16 81.25%
<!-- Total: 147 152 96.71% -->
Files with Coverage Reduction New Missed Lines %
src/Domain/Creators/DomainCreator.hpp 1 77.78%
src/Evolution/Systems/GeneralizedHarmonic/BoundaryConditions/Outflow.cpp 1 95.45%
src/PointwiseFunctions/AnalyticSolutions/NewtonianEuler/RiemannProblem.cpp 2 98.31%
src/PointwiseFunctions/AnalyticSolutions/Xcts/ConstantDensityStar.cpp 3 77.19%
<!-- Total: 7 -->
Totals Coverage Status
Change from base Build 1356110528: 0.02%
Covered Lines: 48739
Relevant Lines: 52100
💛 - Coveralls