Pressio / pressio

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

rom: trial subspace inconsistency #464

Closed fnrizzi closed 1 year ago

fnrizzi commented 1 year ago

@mzuzek pointed out:

There might be a slight inconsistency in TrialColumnSubspace between spanning set hardcoded as column space:

bool isColumnSpace() const { return true; }
bool isRowSpace()    const { return false; }

and stored in linSpace_:

const std::size_t dimension() const { return linSpace_.dimension(); }

If the spanning set is hardcoded, dimension() should assume columns too - instead of asking linSpace_ (which becomes pretty useless in that case...). If not, isColumnSpace() and isRowSpace() should ask linSpace_ too.

fnrizzi commented 1 year ago

i don't think this is an inconsistency because by definition a TrialColumnSubspace is a column space. Hardiwring true/false is just an implementation detail. If one constructs a TrialColumnSubspace with a row-space basis then it breaks the precondition: https://pressio.github.io/pressio/components/rom_trial_column_subspace.html#preconditions

Regardin the dimension, we don't have access to the matrix we only have access to the linSpace_, we could set the dimension from the constructor but i don't see the advantage. Also, right now the class only has constructors accepting a matrix object, but this is temporary because we need to also support constructors accepting a linear subspace object. In that case, we really don't have access to the basis so the dimension must be queried from the linSpace.

Last thing: we could also hardwire the dimension but linSpace is never useless because linSpace is the thing that contains the basis , so we cannot access the basis if we did not have linSpace