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

Simplify VectorArray implementations #1584

Merged
merged 19 commits into from Mar 8, 2022
Merged

Simplify VectorArray implementations #1584

merged 19 commits into from Mar 8, 2022

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented Feb 22, 2022

This refactors VectorArray to defer all interface methods to operations on an internal VectorArrayImpl object. VectorArray itselfs manages copy-on-write semantics, the handling of product arguments, error checking and the handling of views.

Additionally the following changes have been made:

  • Internal access to the VectorObjects stored in a ListVectorArrays is now provided via the vectors property. The old name _list is deprecated.
  • The individual sub-blocks of a BlockVectorArray can be accessed via the blocks property. No copy is made. The blocks method and the num_blocks property have been deprecated. To ensure that the user does not append to/delete from blocks they have accessed via U.blocks each method of BlockVectorArrayImpl now asserts _blocks_are_valid which produces a helpful error message.

@sdrave sdrave added the pr:change Change in existing functionality label Feb 22, 2022
@sdrave sdrave added this to the 2022.1 milestone Feb 22, 2022
@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #1584 (2763aeb) into main (6610dae) will increase coverage by 0.14%.
The diff coverage is 87.71%.

Impacted Files Coverage Δ
src/pymor/algorithms/projection.py 100.00% <ø> (ø)
src/pymor/models/mpi.py 75.60% <55.55%> (-2.32%) ⬇️
src/pymor/operators/mpi.py 78.53% <58.33%> (-1.60%) ⬇️
src/pymor/bindings/ngsolve.py 79.19% <66.66%> (ø)
src/pymor/vectorarrays/mpi.py 63.36% <75.96%> (+6.75%) ⬆️
src/pymor/bindings/fenics.py 83.72% <80.00%> (-0.24%) ⬇️
src/pymor/vectorarrays/interface.py 90.41% <90.74%> (+7.44%) ⬆️
src/pymor/vectorarrays/block.py 90.27% <91.58%> (-1.09%) ⬇️
src/pymor/operators/block.py 93.71% <92.30%> (ø)
src/pymor/vectorarrays/list.py 83.72% <93.97%> (-1.41%) ⬇️
... and 14 more

@sdrave sdrave marked this pull request as ready for review March 2, 2022 17:23
@sdrave sdrave requested a review from a team March 2, 2022 17:23
:class:`BlockVectorArray` can be used in conjunction with
:class:`~pymor.operators.block.BlockOperator`.
"""
class BlockVectorArrayImpl(VectorArrayImpl):
Copy link
Member

Choose a reason for hiding this comment

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

Is this class referenced elsewhere in a docstring? Should it get one that maybe just refers to the VectorArrayImpl?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it isn't. I don't think that adding a docstring here is necessary since a regular user should not be exposed to this class. I'd rather require docstrings also for internal classes.

:class:`~pymor.bindings.ngsolve.NGSolveVector` and
:class:`~pymor.bindings.ngsolve.NGSolveVectorSpace`.
"""
class ListVectorArrayImpl(VectorArrayImpl):
Copy link
Member

Choose a reason for hiding this comment

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

Same as BlockVectorArrayImpl

Copy link
Member Author

Choose a reason for hiding this comment

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

See my response above.

return self.__class__([v.conj() for v in self._list], self.space)

class ListVectorArray(VectorArray):
"""|VectorArray| implemented as a Python list of vectors.
Copy link
Member

Choose a reason for hiding this comment

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

Refer to the Impl here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

For similar reasons as above, I'd prefer to not do it.

@renefritze
Copy link
Member

Same comments for the MPI and numpy arrays too.

@ftalbrecht
Copy link
Contributor

Looks good. However, could you give a brief motivation for this change? Do I understand it correctly that more work is done in the base classes and less work needs to be done in the actual implementations (bluntly speaking)?

@sdrave
Copy link
Member Author

sdrave commented Mar 3, 2022

Looks good. However, could you give a brief motivation for this change? Do I understand it correctly that more work is done in the base classes and less work needs to be done in the actual implementations (bluntly speaking)?

The main reason is that the current implementations are quite complicated and it seems daunting to implement a new VectorArray. Also, we are yet again discussing attaching products to VectorSpaces. After merging this PR that should require only changes to VectorArray but not the implementations.

@sdrave sdrave merged commit ef3242c into main Mar 8, 2022
@sdrave sdrave deleted the simplify_vectorarray_impl branch March 8, 2022 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:change Change in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants