Pressio / pressio

core C++ library
Other
45 stars 7 forks source link

`pressio/utils`: cleanup what is either unused or move elsewhere #677

Closed fnrizzi closed 3 weeks ago

fnrizzi commented 1 month ago
  1. identify things that are unused/better off moving them

    • pressio requires c++17, so the make_unique that we have in here should be removed and replaced with the std one.
    • there are other things that are not used or should be moved
  2. make a list of things with the proposed fix

  3. make separate PRs for each fix

cwschilly commented 1 month ago

@fnrizzi Here is a brief survey of where all of the utils are used:

Constants

make_unique

InstanceOrReferenceWrapper

NoOperation

TeuchosPerformanceMonitor

read_ascii_matrix_stdvecvec()

cwschilly commented 1 month ago

@fnrizzi Here is my plan for approaching this issue:


Constants

- NOTES:
    - the integers return `static_cast<scalar_t>(#)`
    - the fractions are used...
        - oneHalf() - nowhere
        - negOneHalf() - ode
        - oneOvThree() - ode_constants.hpp
        - twoOvThree() - ode_constants.hpp
        - fourOvThree() - ode_constants.hpp
        - threeOvTwo() - ode
    - small note: ode_explicit_stepper_with_mass_matrix.hpp defines its own fractions:

            constexpr auto oneOvThree = one/three;
        constexpr auto twoOvThree = two/three;
        constexpr auto threeOvFour = three/four;
        constexpr auto fourInv = one/four;

            (where one, two, three, and four are from the Constants struct)

- PLAN:
    - All of the fractions will be moved to ode_constants.hpp (and used in ode)
    - All of the integers will be be `static_cast` in place (in both pressio and pressio-ops)

- PROS:
    - Avoid duplicating `Constants` struct across repos
    - static_cast<scalar_t>(0) is shorter than
      ::pressio::utils::Constants<scalar_type>::zero();

- CONS:
    - `static_cast<scalar_t>(0)` might be slightly less readable than `::pressio::utils::Constants<scalar_type>::zero()` (although not by much)

make_unique

- PLAN: Replace with std::make_unique

InstanceOrReferenceWrapper

- PLAN: Move to include/pressio/solvers_nonlinear

- PROS:
    - ode depends on solvers_nonlinear, so it would still be able to access it

- CONS:
    - the class isn't directly related to solvers at all, so it might not make sense to put it in this directory

NoOperation

- NOTES
    - Only the <void> overload is used
    - It's only used in one file (lspg_steady.hpp)
    - It's small enough (9 lines) that we could probably add it in the file

- PLAN: 2 options
    1) Move entire noop.hpp file into rom directory if we plan to use it more in the future
    2) Just define the <void> overload at the top of lspg_steady.hpp if that's the only place we plan to use it

TeuchosPerformanceMonitor

- NOTES
    - This isn't used anywhere, and looks like it hasn't been used for a while
    - One use I found is from 5 years ago at the end of a nonlinear solvers test

- PLAN: Remove

- PROS:
    - No longer used
    - When it was used, it was optional (#IFDEF HAVE_TEUCHOS_TEUCHOS) and did not affect the unit test if it was on or off

- CONS:
    -  Could be useful in the future if we want to compare timer results between a unit test and its known/accepted timings

read_ascii_matrix_stdvecvec

- NOTES:
    - Similar to TeuchosPerformanceMonitor, this function hasn't been altered for three years
        - And even that was a small formatting change--it hasn't been meaningfully changed for five years

- PLAN: Remove

- PROS:
    - I can't find any time this has been used in Pressio (I've searched from various commits dating 3-5 years ago)

- CONS:
    - It is a useful utility (I've actually used it while working on another project)
    - Maybe it is used elsewhere in the Pressio ecosystem?
fnrizzi commented 3 weeks ago

@cwschilly what do we have left here to deal with?