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 compute() to Model #1113

Merged
merged 6 commits into from Oct 12, 2020
Merged

add compute() to Model #1113

merged 6 commits into from Oct 12, 2020

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented Oct 6, 2020

With this PR, Model gets a new compute method which realizes the computation of the model's solution and of derived quantities such as outputs or error estimates.

  • solve, output and estimate_error are implemented by calling compute with appropriate arguments.

  • By relying on a central method, we can avoid having to pass computed values from the outside, e.g. the solution to estimate_error. (Note that it might depend on the Model which values have to be passed.)

  • compute returns a dict of all computed values. solve, etc. remain as convenience methods.

  • compute can be easily extended to return further quantities of interest. E.g. an optional stages argument might indicate to return intermediate Newton stages as an additional item of the return value.

  • Model.compute has a default implementation which relays individual computational tasks to dedicated _compute_solution, _compute_output, etc. methods which allow to more easily customize compute when subclassing. Before these methods are called _compute is called in which, e.g., simultaneous solution and output computation can be realized. For values that already have been computed, the respective _compute_... methods will then be skipped.

  • The signature of compute should stay stable for the foreseeable future. The base class implementation may change in the future.

  • I decided to keep automatic caching of the solution in the default implementation of compute. In particular, for models using _compute_solution, a call to m.output(mu) will not trigger an additional solution of the model, when m.solve(mu) has been called before for the same mu and caching is enabled.

  • The solve_d_mu and output_d_mu methods from Towards linear optimization (dual problem, sensitivity problem, output gradient) #1110 will also make use of compute.

See discussion in #1095.

Documentation still needs to be updated.

@pymor/pymor-devs, @HenKlei, @TiKeil, while not much has changed internally, this PR will significantly affect the user expercience. Please take a close look.

@sdrave sdrave added this to the 2020.2 milestone Oct 6, 2020
@sdrave sdrave added the pr:change Change in existing functionality label Oct 6, 2020
@TiKeil
Copy link
Contributor

TiKeil commented Oct 6, 2020

For me this looks very good.

@HenKlei
Copy link
Member

HenKlei commented Oct 9, 2020

Since the parameter.parse call now happens in compute, the part in the tutorial (lines 161 to 169 in the original tutorial) where parameters.parse is missing should be adjusted.

@sdrave sdrave marked this pull request as ready for review October 9, 2020 09:41
@sdrave
Copy link
Member Author

sdrave commented Oct 9, 2020

Since the parameter.parse call now happens in compute, the part in the tutorial (lines 161 to 169 in the original tutorial) where parameters.parse is missing should be adjusted.

Done.

@sdrave
Copy link
Member Author

sdrave commented Oct 9, 2020

This should now be ready to be merged.

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #1113 into master will decrease coverage by 0.05%.
The diff coverage is 59.21%.

Impacted Files Coverage Δ
src/pymor/models/iosys.py 45.34% <ø> (-0.02%) ⬇️
src/pymor/models/interface.py 65.21% <52.30%> (-21.97%) ⬇️
src/pymor/algorithms/error.py 88.60% <100.00%> (+0.07%) ⬆️
src/pymor/algorithms/greedy.py 85.45% <100.00%> (ø)
src/pymor/models/basic.py 85.71% <100.00%> (+5.45%) ⬆️
src/pymor/models/mpi.py 27.02% <100.00%> (ø)
src/pymor/models/neural_network.py 94.44% <100.00%> (+6.34%) ⬆️
src/pymor/vectorarrays/list.py 84.19% <0.00%> (-0.20%) ⬇️

@HenKlei
Copy link
Member

HenKlei commented Oct 9, 2020

Since the parameter.parse call now happens in compute, the part in the tutorial (lines 161 to 169 in the original tutorial) where parameters.parse is missing should be adjusted.

Done.

I meant this part:

That did not work too well! In pyMOR, all parametric objects expect the
`mu` argument to be an instance of the :class:`~pymor.parameters.base.Mu`
class. :meth:`~pymor.models.interface.Model.solve` is an exception: for
convenience, it accepts as a `mu` argument anything that can be converted
to a :class:`~pymor.parameters.base.Mu` instance using the
:meth:`~pymor.parameters.base.Parameters.parse` method of the
:class:`~pymor.parameters.base.Parameters` class. In fact, if you look
back at the implementation of :meth:`~pymor.models.interface.Model.solve`,
you see the explicit call to :meth:`~pymor.parameters.base.Parameters.parse`.

@sdrave
Copy link
Member Author

sdrave commented Oct 9, 2020

Since the parameter.parse call now happens in compute, the part in the tutorial (lines 161 to 169 in the original tutorial) where parameters.parse is missing should be adjusted.

Done.

I meant this part:

That did not work too well! In pyMOR, all parametric objects expect the
`mu` argument to be an instance of the :class:`~pymor.parameters.base.Mu`
class. :meth:`~pymor.models.interface.Model.solve` is an exception: for
convenience, it accepts as a `mu` argument anything that can be converted
to a :class:`~pymor.parameters.base.Mu` instance using the
:meth:`~pymor.parameters.base.Parameters.parse` method of the
:class:`~pymor.parameters.base.Parameters` class. In fact, if you look
back at the implementation of :meth:`~pymor.models.interface.Model.solve`,
you see the explicit call to :meth:`~pymor.parameters.base.Parameters.parse`.

Right ..

@sdrave sdrave merged commit 4461745 into master Oct 12, 2020
2 checks passed
@sdrave sdrave deleted the compute branch October 12, 2020 08:17
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

6 participants