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

Improved parameter handling #923

Merged
merged 42 commits into from
May 28, 2020
Merged

Improved parameter handling #923

merged 42 commits into from
May 28, 2020

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented May 12, 2020

This pull request contains a large amount of improvements and simplifications as to how parameters are handled in pyMOR:

  • New nomenclature: the Parameter class has been renamed to Mu, ParameterType has been renamed to Parameters. Correspondingly, the paramter_type attribute of Parametric has been renamed to parameters. An item of the parameters dict is called a parameter of the Parametric object. Thus, a Mu objects represents a mapping of (free) parameters to concrete values. The i-th 'component of a parameter' is now simply the i-th number in the array to which the parameter is mapped.

  • Parameters are now always one-dimensional NumPy arrays. parameters[p] is now an int specifying the size of the array for parameter p. In particular, scalar parameters p should now be defined as {p: 1} instead of {p: ()}, and the scalar value is retrieved as mu[p][0]. In particular this affects ExpressionParameterFunctional and ExpressionFunction, where the user now has to write p[0] instead of p.

  • Passing Mu instances as mu is now mandatory. Apart from clearly defining, which values are allowed as mu arguments, the implementor of a parametric object no longer has to invoke parse_parameter to ensure that mu is well-formed. At the same time, a mu which is accepted by a given object will almost always also be accepted by any child object. Furthermore, giving mu a fixed type will simplify introducing time-dependent Mus (which will be subject to a later PR). The only exception to the rule are interface methods of Model and Reductors which still allow passing lists of numbers or dicts of numbers/lists.

  • To assert that a given mu is compatible with the object's parameters (previously ensured by calling parse_parameter), the new idiom is assert self.parameters.assert_compatible(mu).

  • Mu instances are now immutable and have a with_ method similar to ImmutableObject.

  • Parametric has been renamed to ParametricObject and is now a proper base class instead of a mixin class.

  • Instead of calling build_parameter_type (which has been moved to the class method Parameters.of) in __init__, the parameters of a ParametricObject are now automatically inferred from the object's __init__ arguments together with parameters_own and parameters_internal properties, which can be set in __init__ to indicate that the object itself introduces new parameters (parameters_own) or assigns fixed values to parameters of it's child objects (parameters_internal). It is still possible to assign to the parameters property in __init__ to override this automatic behavior.

  • The notion of ParameterSpace has been simplified and decoupled from Models: every parameter space is now a CubicParameterSpace, which has been renamed to pymor.parameters.base.ParameterSpace. The pymor.parameters.spaces module has been removed. To instantiate a ParameterSpace, the space method of Parameters can be used. E.g., we can write fom.parameters.space(0.1, 10) to obtain a ParameterSpace for the Parameters of fom, where each parameter can range from 0.1 to 10. Individual ranges can be specified as a dict: fom.parameters.space({'diffusion': (0.1, 10), 'advection': (1, 2)}). Attaching a fixed ParameterSpace to a Model lead to some technical complications (to instantiate the ParameterSpace the exact ParameterType had to be known, which often forced the user to explicitly specify it even though it was derived by the Model, and Reductors had to ensure to pass the space to the ROM). Moreover, often the parameter range of interest is not a fixed property of the Model but depends on the usage of the Model. Consequently, Models no longer have a parameter_space attribute. However, most analyticalproblems still have a ParameterSpace of interest attached.

  • The parameter used for time in timestepping algorithms has been renamed from _t to t.

  • The implementations of Parameters, ParametricObject, Mu and ParameterSpace have been overhauled.

Fixes #869.

sdrave and others added 26 commits April 28, 2020 13:38
except for public methods of Models and sys-mor reductors
remove parse_parameter from Parametric and from_parameter_type from
Parameter in favor of ParameterType.parse. Only use ParameterType.parse
when mu is not a Parameter.
and inherit from ImmutableObject to make it a proper base class instead
of a mixin.
and remove parameters.spaces in favor of new ParameterSpace
class in parameters.base
to agree with new nomenclature
@sdrave sdrave added pr:new-feature Introduces a new feature pr:change Change in existing functionality interfaces labels May 12, 2020
docs/source/tutorial01.rst Outdated Show resolved Hide resolved
@sdrave sdrave marked this pull request as ready for review May 25, 2020 14:21
@sdrave
Copy link
Member Author

sdrave commented May 25, 2020

From my point of view this can now be merged. Any final comments @pymor/pymor-devs, @TiKeil, @HenKlei?

@pmli, I decided to keep the calls to parameters.parse for now. Regarding sample_logarithmically, I propose to open a separate issue/pr.

I also decided to keep the name ProjectionParameterFunctional as I would first like to discuss the naming convention for Operators and ParameterFunctionals in general.

@codecov
Copy link

codecov bot commented May 25, 2020

Codecov Report

Merging #923 into master will not change coverage.
The diff coverage is n/a.

@HenKlei
Copy link
Member

HenKlei commented May 25, 2020

The latest changes look good to me.

@pmli
Copy link
Member

pmli commented May 27, 2020

Is it necessary to put self.parameters.assert_compatible(mu) in an assert statement? It feels a bit redundant to write "assert" twice...

It is also unclear to me how to sample parameters for a model that is not defined by a problem. Should the parameter space be constructed separately?

@sdrave
Copy link
Member Author

sdrave commented May 27, 2020

Is it necessary to put self.parameters.assert_compatible(mu) in an assert statement? It feels a bit redundant to write "assert" twice...

It is not necessary but avoids the unnecessary function call when running with -O.

It is also unclear to me how to sample parameters for a model that is not defined by a problem. Should the parameter space be constructed separately?

Yes. The idea is that in general a Model does not have an inherent parameter range of interest. It may come with an allowed range for the parameters, but this is usually something different, and there is currently no mechanism in pyMOR to enforce parameter ranges. (Of course, you can always put a check in your solve/apply methods.) Getting a space for your model is as simple as writing

m.parameters.space(lower, upper)

or

m.parameters.space({'foo': (lower, upper), 'bar': (lower2, upper2)})

I certainly can be more convenient, to have the space attached to the Model, but if you look at the changes in the demos, you will see that, at least from my POV, the annoyances are very minor.

@ftalbrecht
Copy link
Contributor

Is it necessary to put self.parameters.assert_compatible(mu) in an assert statement? It feels a bit redundant to write "assert" twice...

It is not necessary but avoids the unnecessary function call when running with -O.

I agree with Petars comment, but I would rather rename the method to remove the redundant assert. What about something like

assert self.parameters.is_compatible(mu)

@sdrave
Copy link
Member Author

sdrave commented May 27, 2020

I agree with Petars comment, but I would rather rename the method to remove the redundant assert. What about something like

assert self.parameters.is_compatible(mu)

To me is_compatible would mean that the method returns either True or False but this method actually raises an AssertionError. Note that things are this way to get a useful error message. If is_compatible were to return True/False, the idiom would be

assert self.parameters.is_compatibe(mu), self.parameters.why_incomaptible(mu)

or

assert mu >= self.parameters, self.parameters.why_incomaptible(mu)

which is the actual implementation of assert_compatible. I guess, nobody will know what mu >= self.parameters means, so I can live with the following options:

  • Keep assert self.parameters.assert_compatible(mu)
  • Rename Parameters.__le__ to is_compatible and use
    assert self.parameters.is_compatibe(mu), self.parameters.why_incomaptible(mu)

@ftalbrecht
Copy link
Contributor

I agree with Petars comment, but I would rather rename the method to remove the redundant assert. What about something like

assert self.parameters.is_compatible(mu)

To me is_compatible would mean that the method returns either True or False but this method actually raises an AssertionError. Note that things are this way to get a useful error message.

Ah, I overlooked this part. Sorry

which is the actual implementation of assert_compatible. I guess, nobody will know what mu >= self.parameters means, so I can live with the following options:

* Keep `assert self.parameters.assert_compatible(mu)`

+1

* Rename `Parameters.__le__` to `is_compatible` and use
  ```python
  assert self.parameters.is_compatibe(mu), self.parameters.why_incomaptible(mu)
  ```

-1, I will always forget the 2nd part...

@sdrave
Copy link
Member Author

sdrave commented May 27, 2020

Ok, let's keep assert_compatible. Anybody in favor of replacing __le__ with is_compatible? I think I would be ..

@pmli
Copy link
Member

pmli commented May 27, 2020

I guess is_compatible is better since it's not just a relation between Parameters, but also Mu.

@sdrave sdrave merged commit 6f5b4cb into master May 28, 2020
@sdrave sdrave deleted the improve_parameter_handling branch May 28, 2020 12: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 pr:new-feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make parameter handling less surprising
5 participants