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

adding randomized algorithms #665

Merged
merged 3 commits into from May 8, 2019
Merged

adding randomized algorithms #665

merged 3 commits into from May 8, 2019

Conversation

deneick
Copy link
Contributor

@deneick deneick commented Apr 5, 2019

I added randomized range finder algorithms: one adaptive algorithm and one with power iterations.

@pmli pmli added linear algebra pr:new-feature labels Apr 8, 2019
@pmli
Copy link
Member

@pmli pmli commented Apr 8, 2019

Hi @deneick, this is looking good.

Is there some literature you can reference? If yes, it would go in docs/source/bibliography.rst, which you can then cite in the function docstring.

@deneick
Copy link
Contributor Author

@deneick deneick commented Apr 8, 2019

Hi @pmli, I added the literature.

Copy link
Member

@pmli pmli left a comment

@deneick Below are some (mostly minor) comments.

Some unit tests would be nice. They don't have to be complicated, e.g. checking the probabilistic error bound would be too much. They could just check that the result is a vector array of expected dimensions and dtype.

There are also a few PEP8 errors. You can run flake8 src/pymor/algorithms/randrangefinder.py to see them.

src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
@deneick
Copy link
Contributor Author

@deneick deneick commented Apr 8, 2019

I guess I covered everything.

src/pymor/algorithms/randrangefinder.py Outdated Show resolved Hide resolved
@pmli
Copy link
Member

@pmli pmli commented Apr 8, 2019

Increasing the test coverage would be nice. You could add some simple tests to src/pymortests. E.g.

def test_rrf():
    A = ...
    Q = rrf(A)
    assert Q in A.range
    assert len(Q) == 2

@pmli pmli added this to the 2019.2 milestone Apr 9, 2019
@deneick deneick mentioned this pull request Apr 9, 2019
@pmli
Copy link
Member

@pmli pmli commented Apr 18, 2019

Just a few minor remarks:

  • there is no reference in approximate_operatornorm
  • there are a few flake8 issues

Otherwise, looks good to me. Sphinx documentation is rendering nicely and code coverage is excellent (98%).
@pymor/pymor-devs Any other opinions?

@deneick
Copy link
Contributor Author

@deneick deneick commented Apr 24, 2019

I don't have a reference for approximate_operatornorm, so I added some explanation.

pmli
pmli approved these changes Apr 25, 2019
@pmli pmli requested a review from sdrave Apr 25, 2019
@sdrave
Copy link
Member

@sdrave sdrave commented Apr 26, 2019

Sorry, I need some time to think about this, and I don't have any right now. Will try to submit a review by the end of next week.

@renefritze
Copy link
Member

@renefritze renefritze commented May 6, 2019

FYI: Slycot switched their build system to skbuild in their latest point release and there seems to be no longer wheels for that release. Hence the install fails since skbuild is not a dependency, hence our CI fails. We should just pin it to version 0.3.3. exactly for now.

Copy link
Member

@sdrave sdrave left a comment

Very nice work, @deneick! Only a few minor remarks ..

src/pymor/algorithms/randrangefinder.py Show resolved Hide resolved

def mvinv(v):
return source_product.apply_inverse(source_product.range.from_numpy(v)).to_numpy()
L = LinearOperator((source_product.source.dim, source_product.range.dim), matvec=mv, matmat=mv)
Copy link
Member

@sdrave sdrave May 7, 2019

Choose a reason for hiding this comment

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

Is matmat=mv really correct? I would expect scipy to use column vectors, but from_numpy/to_numpy uses column vectors (@pmli, I know, I know ...).

Copy link
Contributor Author

@deneick deneick May 7, 2019

Choose a reason for hiding this comment

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

I just realized scipy does not use matmat in eigsh, but only in svds, so I will leave this out here.

def mvinv(v):
return source_product.apply_inverse(source_product.range.from_numpy(v)).to_numpy()
L = LinearOperator((source_product.source.dim, source_product.range.dim), matvec=mv, matmat=mv)
Linv = LinearOperator((source_product.range.dim, source_product.source.dim), matvec=mvinv, matmat=mvinv)
Copy link
Member

@sdrave sdrave May 7, 2019

Choose a reason for hiding this comment

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

Same here.

src/pymor/algorithms/randrangefinder.py Show resolved Hide resolved
This is an implementation of Algorithm 4.4 in [HMT11]_.

Given the |Operator| `A`, the return value of this method is the |VectorArray|
`Q` whose vectors form an orthonomal basis for the range of `A`.
Copy link
Member

@sdrave sdrave May 7, 2019

Choose a reason for hiding this comment

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

approximate orthonormal basis

Copy link
Member

@renefritze renefritze left a comment

Please cherry pick this commit to your branch to fix the CI issues with Slycot: 751bc64

return source_product.apply(source_product.source.from_numpy(v)).to_numpy()

def mvinv(v):
return source_product.apply_inverse(source_product.range.from_numpy(v)).to_numpy()
Copy link
Member

@pmli pmli May 7, 2019

Choose a reason for hiding this comment

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

Is adaptive_rrf actually supposed to work if source_product.space.from_numpy is not implemented? Or is from_numpy always implemented for source_products in applications?

Copy link
Member

@sdrave sdrave May 7, 2019

Choose a reason for hiding this comment

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

Or is from_numpy always implemented for source_products in applications?

No, this cannot be expected. In that case you would need to specify lambda_min. For the future, I would hope that at some point we reimplement something like ARPACK in pyMOR. Not sure how much effort would be required, however.

sdrave
sdrave approved these changes May 7, 2019
@sdrave sdrave merged commit e94495b into pymor:master May 8, 2019
4 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants