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

Simplify Reductors and Models #592

Merged
merged 7 commits into from
Feb 27, 2019
Merged

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented Feb 8, 2019

This is non-functional code to give the @pymor/pymor-devs an idea of what I have in mind for the future Reductor design. It's probably more helpful to look at the new reductors/basic.py instead of looking at the diff. Comments welcome ..

As you can see, I have decided to completely remove the projection code from the base class. My impression was that there were too many corner cases to take care of and it might be clearer to explicitly write down the projections.

Fixes #587 when finished.

@sdrave sdrave added sys-mor pr:change Change in existing functionality labels Feb 8, 2019
@sdrave sdrave added this to the 2019.2 milestone Feb 8, 2019
Copy link
Member

@renefritze renefritze left a comment

Choose a reason for hiding this comment

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

Looks generally clearer to me.

rom.disable_logging()
def extend_basis(self, U, basis, method='gram_schmidt', pod_modes=1, pod_orthonormalize=True,
check_orthonormality=None, check_tol=None, copy_U=True):
if check_orthonormality is None:
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional to leave check_orthonormality potentially unset below?
Should this be check_orthonormality = check_orthonormality or (basis in self.products)?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a default for check_orthonormality in the free extend_basis function.

@sdrave
Copy link
Member Author

sdrave commented Feb 11, 2019

BTW, regarding the use of inner products in project: To make things clearer, I could imagine to split project into a project_pg (or some other name) method which is project without the product argument and a project_orth(range_basis, source_basis, product=None, basis_is_orthonormal=False) method, which takes care of the orthogonal projection of an operator onto its image. project could then be deprecated and default implemented as project_orth(range_basis=range_basis, source_basis=source_basis, product=product, basis_is_orthonormal=True).

@pmli
Copy link
Member

pmli commented Feb 11, 2019

Looks ok to me. I guess the idea is to remove GenericePGReductor and add SecondOrderPGReductor etc., right?

I would prefer check_orthonormality to be False by default, since gram_schmidt is usually used before projection.

@sdrave sdrave force-pushed the model_reductor_simplifications branch from 0fa6cad to fde9ccf Compare February 22, 2019 13:41
no longer depend on operators dict
@sdrave sdrave force-pushed the model_reductor_simplifications branch from fde9ccf to 4d487d6 Compare February 22, 2019 16:18
and simplify implementation
@sdrave
Copy link
Member Author

sdrave commented Feb 25, 2019

So, the new Reductors should be working now (except for DelayBHIReductor and TFInterpReductor for which no tests seem to exist).

I have also removed operators from ModelInterface. This makes ModelBase quite simple. In particular, there are no longer any special_operators.

Documentation still needs updating.

@pmli, for now I have added an outputs dict to StationaryModel and InstationaryModel. From the PDE user's POV it seems natural to define several independent output functionals. We can discuss later on if this is desirable or not (to_lti could stack all functionals in a BlockOperator).

@sdrave sdrave requested a review from pmli February 25, 2019 15:00
@pmli
Copy link
Member

pmli commented Feb 25, 2019

So, the new Reductors should be working now (except for DelayBHIReductor and TFInterpReductor for which no tests seem to exist).

Notebooks and demos appear to be working. TFInterpReductor (which should probably be renamed to TF_BHIReductor at some point) is kind of tested in heat.ipynb notebook and delay.py demo since it's used in TF-IRKA. DelayBHIReductor really is untested.

LTIPG and SOLTIPG reductors are a bit noisy, especially in IRKA, and especially when reporting "Assembling error estimator" which I'm pretty sure does nothing (or at least nothing relevant for BT and IRKA).

@pmli, for now I have added an outputs dict to StationaryModel and InstationaryModel. From the PDE user's POV it seems natural to define several independent output functionals. We can discuss later on if this is desirable or not (to_lti could stack all functionals in a BlockOperator).

Agreed.

@sdrave
Copy link
Member Author

sdrave commented Feb 26, 2019

@pmli, ah sorry, TFInterpReductor should be working. DelayBHIReductor cannot work, since it is missing the right PGReductor attribute. Maybe you could fix that as an exercise on working with the new reductor approach 😄?

Regarding the logging: I have added a kind of ugly hack to prevent the estimator assembly log message from being displayed when there is no estimator to assemble. I would like to keep the other log messages in principle as they seem useful to me to see where time is spent. (I would not like to disable them for LTIPGReductor but keep them for the rb reductors as they are basically doing the same thing.) Right now, I am not completely satisfied with the way logging works. Basically I would like to have hierarchical log level s.t. nested log messages are suppressed if the parent messages are not displayed. This should be discussed in a separate issue, however.

@pmli
Copy link
Member

pmli commented Feb 26, 2019

I've added the DelayLTIPGReductor and a notebook which demonstrates DelayBHIReductor.

About logging, I agree about leaving "Operator projection" and "Building ROM" messages. My issue is that it pollutes the table in IRKAReductor, but maybe we can resolve it in another PR (e.g., by restructuring the table or silencing LTIPGReductor).

@sdrave sdrave changed the title [WIP] Simplify Reductors and Models Simplify Reductors and Models Feb 27, 2019
@sdrave
Copy link
Member Author

sdrave commented Feb 27, 2019

@pmli, regarding logging: I think there is more important stuff right now than improving the logging, so I would go for 'e.g., by restructuring the table' ..

@sdrave sdrave merged commit 4654bcd into master Feb 27, 2019
@sdrave sdrave deleted the model_reductor_simplifications branch February 27, 2019 15:24
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.

3 participants