Add regularization to synthesis methods via penalty_function#383
Add regularization to synthesis methods via penalty_function#383dherrera1911 wants to merge 107 commits intomainfrom
penalty_function#383Conversation
43b016b to
9d6ab2a
Compare
|
I see some failings from |
|
Hey @billbrod I think the PR is ready for a code review, everything on my TODO list is done with for this PR. There seem to be some errors in the automatic checks, having to do with the
|
billbrod
left a comment
There was a problem hiding this comment.
Thanks for your work Daniel!
Looking through this, I'm realizing I got confused as to where we ended up with partials. From what you have here, my understanding is, in order to change the allowed_range, users will have to either use partials or define their own custom penalty like
def custom_penalty(x):
return po.tools.regularization.penalize_range(x, allowed_range=(.2, .8))If they use partials, they will be unable to save/load, right (because of the issue with _get_name)? In that case, I do not think we should tell people to use partials, since that's a weird sharp edge.
Instead, I think the docs / examples should show people how to write a custom penalty like above. Advantage here, is it should be easy to see that you can easily combine two penalties in that new function (and the docs should say so, explicitly).
The reason to use partials is if you're lazy (like me) and want to do it in the same line where you pass it to the init method:
po.synth.Metamer(
img,
model,
penalty_function=functools.partial(po.tools.regularization.penalize_range, allowed_range=(0.0, 255.0)
)And I don't think that's worth telling people about -- advanced users will realize they can do that based on the custom def example, and otherwise we should explain (or at least link) to what partials mean for non-advanced users
Other notes:
- I'm not sure what to do for the description of
penalty_functionin the docstrings. I don't think "It constrains certain properties of the metamer" is that informative, but without that sentence, it reads fairly abstract. What about something like "For example, this can penalize all values that lie outside of a specified range"? - Need to add a paragraph to the
reproducibility.mddocs page about the change to penalty_function. should be pretty straightforward, more or less a copy of the existing paragraph underFutureWarning in load in plenoptic 1.4 - Metamer and MADCompetition's init docstrings should have examples setting penalty_function
For the failing PS synthesis tests -- yes, I'll go ahead and update these once we've otherwise finished with this PR.
src/plenoptic/tools/validate.py
Outdated
| UserWarning | ||
| If ``input_tensor`` is not 4d. | ||
| If ``input_tensor`` is not 4d, or if ``input_tensor`` has values | ||
| outside (0, 1). |
There was a problem hiding this comment.
split this into two UserWarning. that's the proper way for numpydoc, I believe.
There was a problem hiding this comment.
You mean like:
Warns
-----
UserWarning
If ``input_tensor`` is not 4d.
If ``input_tensor`` has values outside (0, 1).
Or
Warns
-----
UserWarning
If ``input_tensor`` is not 4d.
UserWarning
If ``input_tensor`` has values
outside (0, 1).
Also, I think I wrote it like this following validate_model that has
UserWarning If ``model`` is in training mode or returns an output with other than 3 or 4 dimensions.
There was a problem hiding this comment.
The second. (see the ssim docstring for an example)
And oh you're totally right. Can you change validate_model as well?
| >>> po.tools.validate.validate_input(po.data.einstein()) | ||
|
|
||
| Intentionally fail: | ||
| Raise warning: |
There was a problem hiding this comment.
instead, have this intentionally fail because of the dtype
There was a problem hiding this comment.
You mean, change "Raise warning:" for "Intentionally fail because of the dtype:"?
There was a problem hiding this comment.
no, I mean have this block still intentionally fail, but for a different reason: because the dtype is an int.
penalty_functionpenalty_function
81cd60b to
cc8b79c
Compare
for more information, see https://pre-commit.ci
This reverts commit 7169fdc.
for more information, see https://pre-commit.ci
|
@billbrod I pushed the last edits, addressing everything... but I made a huge mess with git, as you might see. If you can help me fix this on a call, that'd be great. |
Okay, let's find a time to meet next week. I sent you an email to find a time. |
Describe the change in this PR at a high-level
This PR adds basic functionality to include regularization in the synthesis objects
MetamerandMADCompetitionLink any related issues, discussions, PRs
This closes Issue #363
Outstanding questions / particular feedback
penalty_functionis passed? E.g. requiring more than one inputpenalty_functionto use as an example.Describe changes
range_penalty_lambdaandallowed_rangeas inputs toMetamerandMADCompetitionpenalty_functionandpenalty_lambdapenalty_functiondefaults to the old behaviorallowed_rangeinvalidate_inputallowed_rangein some casespo.tools.regularization, movedpenalize_rangethere frompo.tools.optimChecklist
Affirm that you have done the following:
docs/, or (for large changes) adding new files to thedocs/folder.