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

Update all proximal operators in place #16

Merged
merged 1 commit into from
Jun 24, 2019
Merged

Conversation

fred3m
Copy link
Collaborator

@fred3m fred3m commented Jun 22, 2019

No description provided.

@fred3m fred3m requested a review from pmelchior June 22, 2019 03:21
@pmelchior
Copy link
Owner

pmelchior commented Jun 24, 2019

All changes are OK. However, I wonder if we still need to inline change, which is at least somewhat unexpected. We introduced that to allow on-the-fly resizing without interrupting the algorithms in this package, and we're not using this feature any more. The more transparent approach would be to return the modified variable but leave the original intact. Plus, if we actually want to differentiate through those proxes, we couldn't do inline updates either.

So the question here is, should we use this PR to make that change?

@pmelchior
Copy link
Owner

Ignore request, this should be an independent PR.

@pmelchior pmelchior merged commit e562435 into master Jun 24, 2019
@pmelchior pmelchior deleted the tickets/DM-20128 branch June 24, 2019 03:08
@fred3m
Copy link
Collaborator Author

fred3m commented Jun 24, 2019

While the other approach would be more transparent there are a few things that we need to consider:

  • This approach would use more memory, and in light of Check unnecessary copies of data structures scarlet#101 I think we all want to do a better job managing our memory use
  • If we start applying our proximal operators inside the gradients we need to be very careful about how we do this. autograd doesn't like inline updates, but I'm also not sure how it would respond to rewriting the entire array it is using for the gradient. This makes me think that we should add a few more tests to scarlet to make sure that proximal operators and autograd are compatible (which they likely aren't in their current or proposed state).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants