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

From Operator to NumPy operator #238

Merged
merged 15 commits into from Jun 9, 2016

Conversation

Projects
None yet
5 participants
@pmli
Copy link
Member

pmli commented Apr 28, 2016

See #234

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 28, 2016

Coverage Status

Coverage decreased (-0.07%) to 33.542% when pulling 84ade54 on pmli:to_numpy_operator into 8500dc2 on pymor:master.

@sdrave sdrave added this to the 0.5 milestone Apr 28, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 28, 2016

Coverage Status

Coverage decreased (-0.09%) to 33.528% when pulling dd44796 on pmli:to_numpy_operator into 8500dc2 on pymor:master.

@sdrave

This comment has been minimized.

Copy link
Member

sdrave commented May 3, 2016

Thanks for the nice work! I only have two remarks:

  1. The suite for LincombOperator will not work when the operator is parametric, in which case you would need to call op.evaluate_coefficients(mu) to obtain the linear coefficients. I would say one should at least check that all coefficients are numbers and not ParameterFunctionals. Even nicer would be to add an optional mu parameter to _to_matrix and pass it along.
  2. I would prefer to leave out the to_numpy_operator function and rename _to_matrix to to_matrix. This would make it clearer that by using this function you leave the realm of pyMOR's abstractions and directly work on the data. - If you really need the matrix again as an Operator, it should not be too much of a pain to explicitly instantiate the NumpyMatrixOperator. Hoever, others (@ftalbrecht, @andreasbuhr) might disagree? Do you use to_numpy_operator yourself very often?
@pmli

This comment has been minimized.

Copy link
Member Author

pmli commented May 3, 2016

I actually noticed that I kept using ._matrix after to_numpy_operator, so I agree about renaming it.

@andreasbuhr

This comment has been minimized.

Copy link
Contributor

andreasbuhr commented May 4, 2016

Hi Petar,

the code looks good! :-)

I agree: The name to_matrix is more clear.

@ftalbrecht

This comment has been minimized.

Copy link
Member

ftalbrecht commented May 4, 2016

Thx for your work! I agree with Stephan that working with data structures directly (as opposed to operators and vector arrays) means leaving the (current) realm of pyMOR abstractions, and the payment for return shall be the extra instantiation of a NumpyMatrixOperator :). I would thus also vote for the to_matrix variant (at least for the time being).

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 10, 2016

Coverage Status

Coverage increased (+47.3%) to 80.947% when pulling 070aa56 on pmli:to_numpy_operator into 8500dc2 on pymor:master.

op
Operator.
format
Format of the resulting |SciPy sparray|.

This comment has been minimized.

@sdrave

sdrave Jun 8, 2016

Member

Do you really mean sparray? Never heard of that before. Also, |foo| is a subtitution in reST (http://www.sphinx-doc.org/en/stable/rest.html#substitutions). We define all our substitions in docs/source/substitutions.py.

This comment has been minimized.

@pmli

pmli Jun 8, 2016

Author Member

Whoops! Thanks for noticing!

return _to_matrix(op, format, mapping, mu)


def _to_matrix(op, format, mapping, mu):

This comment has been minimized.

@sdrave

sdrave Jun 8, 2016

Member

It might be good to do op = op.assemble(mu) as a first step. This will give you a new operator which often is already as close to a matrix as possible (take a look at the implementations of assemble in operators.constructions and operators.numpy). Calling assemble might be useful to catch some cases (think of external solvers) you are unable to handle. On the other hand, your code tries harder to convert operators to matrices than assemble does, so both have their justification. (I guess the semantics of assemble are not completely clear at the moment, but the rough meaning might be 'try to precomute as much as reasonably possible, given a fixed parameter mu.)

@sdrave

This comment has been minimized.

Copy link
Member

sdrave commented Jun 8, 2016

Apart from the two minor remarks above, I think this is ready to get merged?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 8, 2016

Coverage Status

Coverage increased (+43.2%) to 76.776% when pulling 43cc916 on pmli:to_numpy_operator into 8500dc2 on pymor:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 8, 2016

Coverage Status

Coverage decreased (-0.03%) to 80.983% when pulling 1b1542d on pmli:to_numpy_operator into 0ae424a on pymor:master.

@sdrave sdrave merged commit 07d0d49 into pymor:master Jun 9, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 80.983%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdrave

This comment has been minimized.

Copy link
Member

sdrave commented Jun 9, 2016

FYI, I have updated AUTHORS.md and added

algorithms.to_matrix

to your contributions list. Thanks again for your work!

@pmli pmli deleted the pmli:to_numpy_operator branch Jun 9, 2016

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