Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate dot/pairwise_dot in favor of inner/pairwise_inner #1066

Merged
merged 2 commits into from Aug 26, 2020
Merged

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented Aug 18, 2020

As proposed in #930, this PR deprecates the dot and pairwise_dot methods of VectorArray which are equivalent to calling inner or pairwise_inner without a product argument.

Any reservations @pymor/pymor-devs?

Note that currently we don't explicitly state anywhere that inner without a product is the Euclidean inner product. I would like to keep it that way as there might be use cases, where you want to implement inner with some 'special' product. (We already had several discussion about removing the product arguments and equipping VectorSpaces with products. I'm still not convinced that this is the right thing to do, but I at least want to have an open door for hacks in that direction.)

@sdrave sdrave added the pr:deprecation Deprecates existing functionality label Aug 18, 2020
@sdrave sdrave added this to the 2020.2 milestone Aug 18, 2020
@sdrave sdrave requested review from renefritze and pmli August 18, 2020 16:06
@pmli
Copy link
Member

pmli commented Aug 19, 2020

Ok, now I see that issue you were referring to in one of the meetings: every subclass of the VectorArray would have to include the if product is not None: branch. Are you fine with it?

@sdrave
Copy link
Member Author

sdrave commented Aug 21, 2020

Ok, now I see that issue you were referring to in one of the meetings: every subclass of the VectorArray would have to include the if product is not None: branch. Are you fine with it?

That's totally fine. However, I am starting to be increasingly concerned about my statement

that currently we don't explicitly state anywhere that inner without a product is the Euclidean inner product. I would like to keep it that way as there might be use cases, where you want to implement inner with some 'special' product. (We already had several discussion about removing the product arguments and equipping VectorSpaces with products. I'm still not convinced that this is the right thing to do, but I at least want to have an open door for hacks in that direction.)

If inner was only used to compute inner products between vectors, that would be totally fine. However, many Operators actually map to dual spaces, so the vector returned by apply actually contain coefficients w.r.t. the dual basis. When using inner (or dot) to compute the dual pairing (which for instance happens when doing Galerkin projection of an Operator representing, say, the weak Laplacian), even if there was a product attached to the space, this product may not be used to compute the dual pairing. So I see two options:

  1. Explicitly state that U.inner(V) has to be equivalent to sum(U.dofs([i]) * V.dofs([i]) for i in range(U.dim) and largely keep things as is.

  2. Embrace the idea that VectorSpaces come with products and distinguish between vectors and co-vectors.

  3. would have the benefit that we could stop passing product arguments in most places. However, it would also mean exposing users to the concept of duality and it would certainly introduce some technical difficulties. For that reason we have rejected the idea multiple times. However, I still find it intriguing enough to give it another shot. What do you think, @pymor/pymor-devs, @TiKeil?

I would like to decide how to proceed in that regard before merging this PR as 2. might have some further consequences regarding inner and/or dot. (In particular, if we were to introduce the concept of duality and let the method, depending on the type of vectors compute the inner product or the dual pairing, dot would seem a better name to me than inner.)

Copy link
Member

@renefritze renefritze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to rebase on master to get rid of the install_checks stage error on gitlab.

@renefritze
Copy link
Member

I would currently favor option 1.

My last experiment with attaching a dtype to vectorspaces brought up enough issues that not-passing product ars around is not a good-enough upside to try something somewhat similar right now.

@sdrave
Copy link
Member Author

sdrave commented Aug 26, 2020

My last experiment with attaching a dtype to vectorspaces brought up enough issues that not-passing product ars around is not a good-enough upside to try something somewhat similar right now.

Yes, I thought about this some more. There are two different layers:

  1. Allowing inner/pairwise_inner without a product argument to be a different inner product than the Euclidean inner product of the DOFs. Since we still need the dual pairing between vectors and co-vectors at various places, we would have to have an additional method for that, or have an additional attribute on the space telling us, whether the vectors a co-vectors or not.

  2. Often we have different inner products around. A typical example would be a parabolic problem, where one might consider the H1-product or/and the L2-product. So if we wanted to get rid of all the product arguments, we would need to be able to change the inner product associated with inner.

Doing only 1. would still force us to keep the product arguments in order to support situations with multiple inner products of interest. At the same time, we would need to formally introduce co-vectors somehow in pyMOR. While this might actually improve clarity in some places, it would be a lot of hassle and confuse a lot of users.

Also doing 2. probably results in a nightmare when Operators come into play. Say I have a Model where all Operators have a common source/range. Now I change the space of a VectorArray by exchanging the product, in order to call pod on it or something else. Should I still be able to apply the Operator to it. What product should be associated with the return value. I think there will be no good answers, and it will all be a mess.

So overall, I decided to add a commit, which clarifies that inner/pariwise_inner without a product is excpected to be the Euclidean inner product of the DOFs.

BTW, after this has been merged, I would also like to drop l2_norm in favor of norm and remove l1_norm from the interface.

@sdrave sdrave requested a review from renefritze August 26, 2020 11:40
@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

Merging #1066 into master will decrease coverage by 0.06%.
The diff coverage is 76.42%.

Impacted Files Coverage Δ
src/pymor/operators/mpi.py 17.56% <ø> (ø)
src/pymor/reductors/residual.py 64.33% <0.00%> (ø)
src/pymor/vectorarrays/mpi.py 31.48% <30.76%> (-1.70%) ⬇️
src/pymor/vectorarrays/interface.py 80.25% <53.84%> (-2.51%) ⬇️
src/pymor/operators/constructions.py 84.83% <90.00%> (ø)
src/pymor/vectorarrays/numpy.py 85.00% <93.75%> (-0.13%) ⬇️
src/pymor/vectorarrays/list.py 84.74% <94.11%> (+0.08%) ⬆️
src/pymor/algorithms/genericsolvers.py 78.35% <100.00%> (ø)
src/pymor/algorithms/line_search.py 96.15% <100.00%> (ø)
src/pymor/algorithms/lrradi.py 97.53% <100.00%> (ø)
... and 8 more

@sdrave sdrave merged commit 5dcd0b7 into master Aug 26, 2020
2 checks passed
@renefritze renefritze deleted the remove_dot branch August 26, 2020 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:deprecation Deprecates existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants