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

reconsider 'options' parameter for OperatorInterface.apply_inverse #122

Closed
sdrave opened this Issue Jun 17, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@sdrave
Member

sdrave commented Jun 17, 2015

No description provided.

@sdrave sdrave added this to the 0.4 milestone Jun 17, 2015

@ftalbrecht

This comment has been minimized.

Member

ftalbrecht commented Jun 18, 2015

Agreed. We should drop the argument for usual operators and use the with_ method to create a new operator with different options. How to proceed with LincombOperator is yet unclear...

@sdrave

This comment has been minimized.

Member

sdrave commented Jul 4, 2015

Ok, so this is decided. I guess, there should be a standardized attribute which holds the options that are being used? How should we name it and what happens with invert_options?

@ftalbrecht

This comment has been minimized.

Member

ftalbrecht commented Jul 7, 2015

This is a good point, did you consider it any further yet? At the moment I would tend to leave the invert_options mostly as they are, since it still makes sense to provide the knowledge of how to invert an operator together with the operator. It would then be possible to obtain a new operator by calling

new_op = op.with_(op.invert_options['bicgstab'])

If the naming is too confusing we could also rename them to available_invert_options and store the current ones in _invert_options or _current_invert_options, but I do not have a strong opinion there.

@sdrave

This comment has been minimized.

Member

sdrave commented Jul 7, 2015

Thinking about it, I believe that invert_options is not such a great name, anyhow. I would propose to use solver_options for the options in use (I could also live with apply_inverse_options) and available_solver_options (or available_apply_inverse_options) for what is currently named invert_options.

From my point of view, it would also be attractive to remove the current invert_options completely from pyMOR. At least for constructions like LincombOperator it would be very hard to provide a list. For everything NumPy-based, the defaults in pyMOR are already a good documentation of what is available. I myself have never used invert_options and I had no fun implementing it ..

@ftalbrecht

This comment has been minimized.

Member

ftalbrecht commented Jul 7, 2015

I agree on the first point (and would probably suggest apply_inverse_options, if we go down that route). I also see the burden of implementing these options and could live with their removal, given that we have not used them before.

However, I still think that it could become very important to have different versions of the same operator with different apply_inverse_options. If we dropped those and went the default route, do you see a way to still achieve that?

We could let an operator have its own options, if he was created with_ them. However, it could become unclear then why this operator behaves differently than the same one without specified options (thus taking the default).

We could also let each operator hold solver_options or apply_inverse_options. If those are set to default, the defaults from defaults will be used, and other options otherwise.

These are just some thoughts and suggestions, though ...

@sdrave

This comment has been minimized.

Member

sdrave commented Jul 7, 2015

@ftalbrecht, I only propose to remove the list of available options. The options set for a specific Operator should be available and settable during creation of the operator.

@sdrave

This comment has been minimized.

Member

sdrave commented Jul 7, 2015

Another question is how to proceed with least squares solvers.

@sdrave

This comment has been minimized.

Member

sdrave commented Nov 19, 2015

See changes in #184, #185.

@sdrave sdrave closed this Nov 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment