Navigation Menu

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

Jump to bottom

ENH: add test for x in op.domain in all solvers, closes #291 #502

Merged
merged 1 commit into from Aug 15, 2016
Merged

ENH: add test for x in op.domain in all solvers, closes #291 #502

merged 1 commit into from Aug 15, 2016

Conversation

adler-j
Copy link
Member

@adler-j adler-j commented Aug 11, 2016

Added input sanitation to the solvers.

@kohr-h
Copy link
Member

kohr-h commented Aug 12, 2016

Good initiative. A few comments:

  • When we raise TypeError, we should print the repr of at least the object that caused the error, so the user sees the provided type against the expected one in the error message. We could also just print the type, which would be a valid alternative.
  • The Newton-type methods should not expect something named grad by default, that's a specific case. Now there was this inconsistency that he quasi-Newton ones take grad and Newton takes op, but IMO this inconsistency should be resolved in the other direction. Actually, I would prefer to keep the names untouched in this PR and do the restructuring separately, see Decide on a (class) structure for solvers #52.

@adler-j adler-j force-pushed the issue-291__x_in_space_solvers branch from db4b3c0 to 7587109 Compare August 15, 2016 08:13
@adler-j
Copy link
Member Author

adler-j commented Aug 15, 2016

Fixed according to comments. Merge?

@adler-j adler-j added the merge? label Aug 15, 2016
kohr-h
kohr-h reviewed Aug 15, 2016
View reviewed changes
odl/solvers/advanced/chambolle_pock.py
@@ -148,13 +148,13 @@ def chambolle_pock_solver(op, x, tau, sigma, proximal_primal, proximal_dual,
"""
# Forward operator
if not isinstance(op, Operator):
raise TypeError('`op` {} is not an instance of {}'
''.format(op, Operator))
raise TypeError('`op` {!r} is not an `Operator`'
Copy link
Member

Choose a reason for hiding this comment

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

We usually write <type> instance. Debatable for sure, but consistency is almost always good :-)

@kohr-h
Copy link
Member

kohr-h commented Aug 15, 2016

See above, otherwise mergable.

@adler-j adler-j force-pushed the issue-291__x_in_space_solvers branch from 7587109 to b51ee49 Compare August 15, 2016 08:33
@adler-j adler-j merged commit 647736a into master Aug 15, 2016
@adler-j adler-j deleted the issue-291__x_in_space_solvers branch August 15, 2016 08:33
@kohr-h kohr-h removed the merge? label Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@adler-j @kohr-h