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

Add sized ctor and public add_operator to Combination #1771

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

greole
Copy link
Collaborator

@greole greole commented Jan 22, 2025

This PR makes the Combination::add_operators member public so that operators can be added after constructing a combination matrix. Additionally, a new constructor is added so that an empty combination can be created first and additional operators are added later.

@ginkgo-bot ginkgo-bot added the mod:core This is related to the core module. label Jan 22, 2025
@upsj
Copy link
Member

upsj commented Jan 22, 2025

Can you describe a use case for this? Combination creation is pretty lightweight, so I am hesitant to add more complexity there.

@greole
Copy link
Collaborator Author

greole commented Jan 22, 2025

Sure and maybe there is a better way to do it. On the OGL side I have std::vector<gko::LinOp> of local and non_local linops representing local and non_local interfaces for Distributed::Matrix. Probably, I could also use this ctr

    explicit Combination(CoefficientIterator coefficient_begin,
                         CoefficientIterator coefficient_end,
                         OperatorIterator operator_begin,
                         OperatorIterator operator_end)

but there are cases (after repartitioning) where operator_begin==operator_end thus it would throw.

@upsj
Copy link
Member

upsj commented Jan 22, 2025

Making the empty case work seems sensible, except for the fact that we don't have anything to base the size on then. BTW ctr is kind of a misleading abbreviation, it could be counter or constructor.

@greole greole changed the title Add sized ctr and public add_operator to Combination Add sized ctor and public add_operator to Combination Jan 22, 2025
@MarcelKoch MarcelKoch self-requested a review February 13, 2025 11:52
@@ -112,6 +112,7 @@ class Combination : public EnableLinOp<Combination<ValueType>>,
add_operators(std::forward<Rest>(rest)...);
}

protected:
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @upsj that we should not expose this member. I think it would be enough to add a constructor, which takes the iterators, and just an dimension as an extra parameter. Then you could build the linop from that parameter, and just check that the operator dimensions match up.

@ginkgo-bot
Copy link
Member

Error: The following files need to be formatted:

include/ginkgo/core/base/combination.hpp
include/ginkgo/core/distributed/preconditioner/schwarz.hpp
include/ginkgo/core/multigrid/multigrid_level.hpp
include/ginkgo/core/multigrid/pgm.hpp
include/ginkgo/core/solver/multigrid.hpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

@greole greole marked this pull request as draft February 19, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod:core This is related to the core module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants