#680: Remove/simplify constants

Pressio / pressio

core C++ library

Other

45 stars 7 forks source link

#680: Remove/simplify constants #685

Closed cwschilly closed 3 weeks ago

cwschilly commented 3 weeks ago

Fixes #680

This PR moves the Constants struct into ode_constants.hpp, so we can still use the fractions (e.g. fourOvThree) easily.

This means that the code in ode is largely unchanged, besides simplifications and replacing utils::Constants with ode::constants::Constants (often with using cnst = ::pressio::ode::constants::Constants<scalar_t>).
All code outside of ode replaces any calls to utils::Constants with static_cast.

fnrizzi commented 3 weeks ago

@cwschilly before i merge it, can you please try to "randomly" pick some places and inject the wrong value (eg.. wheere there is a 1 put 2 or similar) and run the tests to see if these errors are detected? just curious if the test suite is strong enough

cwschilly commented 3 weeks ago

@cwschilly before i merge it, can you please try to "randomly" pick some places and inject the wrong value (eg.. wheere there is a 1 put 2 or similar) and run the tests to see if these errors are detected? just curious if the test suite is strong enough

@fnrizzi You can see the test failures here.

The failures are in ode and rom testing, although my changes were in ode and solvers_nonlinear. That suggests that we might need better coverage at least in solvers_nonlinear.

fnrizzi commented 3 weeks ago

Can you please write/report here in a comment what you changed and what the error was

fnrizzi commented 3 weeks ago

@cwschilly before i merge it, can you please try to "randomly" pick some places and inject the wrong value (eg.. wheere there is a 1 put 2 or similar) and run the tests to see if these errors are detected? just curious if the test suite is strong enough

@fnrizzi You can see the test failures here.

The failures are in ode and rom testing, although my changes were in ode and solvers_nonlinear. That suggests that we might need better coverage at least in solvers_nonlinear.

it is good ode and rom fail. I expect solvers not to fail miserably because what you changed is a coefficient for the levenberg-marquardt parameter, so by changing that we are just perturbing the system a bit but it can recover since it is an iterative method. but i guess to your point, yes we could do a more controlled test.