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

Validate configuration of factory parameters #1785

Open
pratikvn opened this issue Feb 17, 2025 · 4 comments
Open

Validate configuration of factory parameters #1785

pratikvn opened this issue Feb 17, 2025 · 4 comments
Labels
is:enhancement An improvement of an existing feature. is:technical-debt This is aimed at reducing technical debt. mod:core This is related to the core module.

Comments

@pratikvn
Copy link
Member

In many cases, we have factory parameters that are dependent on each other. Currently, we either do not properly validate these dependencies leading to failures, or need to explicitly have logic in the core.

A better approach might be to specify dependencies in the header file when declaring the factory parameters and their default. The dependencies can then be properly validated giving better errors for the user.

@pratikvn pratikvn added is:enhancement An improvement of an existing feature. is:technical-debt This is aimed at reducing technical debt. mod:core This is related to the core module. labels Feb 17, 2025
@upsj
Copy link
Member

upsj commented Feb 17, 2025

This description sounds rather vague, what kinds of dependencies do you mean? Can you give an example in Ginkgo? Parameter A needs to be set to value X to allow parameter B to be used?

@pratikvn
Copy link
Member Author

Yes, I mean dependencies between factory parameters. If parameter A needs to be set for parameter B to be used. Another concrete example in Ginkgo is in the Schwarz preconditioner, where a local_solver and a generated_local_solver cannot be both set.

@MarcelKoch
Copy link
Member

I guess one straight forward issue would be if both with_preconditioner and with_generated_preconditioner are set. IMO both should not be set at the same time.

@upsj
Copy link
Member

upsj commented Feb 17, 2025

I think this issue would benefit from a clearer explanation what kinds of dependencies we actually want to represent, what is currently broken and how it should be changed.

It would theoretically be possible to tackle these at compile-time, e.g. by using std::variant for the parameters and decoupling them from their with_* functions, so multiple functions initialize the same variables. Alternatively, we could have multiple stages of factory_parameters types handling different required parts (e.g. stopping criteria without parameters don't make sense), but that would be quite some complexity. A runtime solution seems more feasible to me, and for that the best way would be in core. So I'm asking should this actually happen in the headers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:enhancement An improvement of an existing feature. is:technical-debt This is aimed at reducing technical debt. mod:core This is related to the core module.
Projects
None yet
Development

No branches or pull requests

3 participants