Plot refactor#438
Open
billbrod wants to merge 59 commits into
Open
Conversation
so can use for animshow
for more informative errors
This reverts commit 1f24944. we're moving this to a separate PR
This reverts commit f576ced. only reverts the parts about the close figure teardown, not the new tests. because the teardown is moved to a separate PR
for displaying eigendistortions, since alpha is also used as kwarg for meaning transparency in other plots, as is more typical for plotting
…lot functions in particular - remove as_rgb arg, since we can infer that (behavior as described in docstring didn't work) - adds process_distortion arg to be more explicit for that - use more internals from synthesis_imshow
unnecessary now
more examples there, showing example with RGB(A) and ImageNet models. also adds new example_eigendistortion_color.pt to registry for use in this example
to avoid the issue of checking for output in doctest
Member
Author
|
Documentation built by Jenkins: https://docs.plenoptic.org/docs/pulls/438/index.html |
having a lot of trouble getting good reproducibility for the curie image, I think because the background is solid black and so that makes it hard to get a good rtol. try einstein instead
omre interesting
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the change in this PR at a high-level
Following up from #418, this restructures a lot of the synthesis-specific plotting code. In that PR, I realized that there was a lot of redundancy (e.g.,
po.plot.mad_pixel_valuesandpo.plot.metamer_pixel_values) were essentially the same function, and that I could make the plotting code either to use by consolidating it. To that end, I have combined many of the synthesis-specific plotting function into singlesynthesis_{plot}functions (e.g.,synthesis_loss) which accept multiple synthesis objects. If the plot cannot accept a given synthesis object (e.g.,synthesis_losscannot accept anEigendistortionobject, because eigendistortions don't have optimization loss), then they raise an informative error message to that effect (and the docstrings/annotations hopefully clarify this). For plotting functions that only operate on a single synthesis object (e.g.,metamer_representation_error), they have been kept separate and retain the naming scheme from #418.Consolidated functions:
synthesis_loss:metamer_loss,mad_losssynthesis_histogram:metamer_pixel_values,mad_pixel_values. Adds capability forEigendistortionand renamed because a histogram is potentially valuable even if the values aren't pixels.synthesis_imshow:metamer_imshow,mad_imshow,eigendistortion_imshow.synthesis_status:metamer_synthesis_status,mad_synthesis_status. Adds capability forEigendistortion.synthesis_animshow:metamer_animshow,mad_animshow.Adds:
histogram: tensor-level histogram plot used bysynthesis_histogram(analogous to how we have animshowthat operates directly on image tensors used bysynthesis_imshow)Remaining functions:
metamer_representation_erroreigendistortion_imshow_allmad_imshow_allmad_loss_allFor giving feedback on how users will interact with these functions, I think the most informative way to review will be to look at the plotting section of the API guide, and look through the docstrings of each of the functions there.
Link any related issues, discussions, PRs
Follow up from #418, closes #417.
Outstanding questions / particular feedback
batch_idxto refer to the first index in the tensors, as image tensors in pytorch typically are 4d:batch, channel, height, width. For eigendistortions, this has a special meaning: the eigenindex (e.g., eigenindex=0 means the distortion corresponds to the first eigenvector). In the shared functions, I've usedbatch_idxas the argument name, describing the relationship in the docstring, whereas I've kept it aseigenindexforeigendistortion_imshow_all(also explaining in the docstring). Is this better than being consistent?synthesis_statusandsynthesis_animshowcreate single figures by combining the outputs of the axis-level functions (synthesis_loss,synthesis_imshow, etc). I have made it so that users can pass arguments to these functions directly using kwargs (hopefully docstrings are clear on this), so they don't clutter up the function signature. Is that clear right now?synthesis_animshow'sylimarg, which controls how we rescale the y-limits of themetamer_representation_errorplot. It is only related to that function, but it only makes sense when animating its output. Therefore, I've kept it as an argument for the animshow function. Is this okay? is there a better way to handle it?synthesis_animshowwasanimatebefore [BREAKING CHANGE] API restructure, adds lazy loading #418, to try and make it more consistent with the tensor-level functions. However, now I'm not sure of that name --synthesis_animshowis the animated analogue ofsynthesis_status, animating that figure over synthesis iteration. But this naming makes it sound like it's the animated analogue ofsynthesis_imshow(aspo.plot.animshowis the animated analogue ofpo.plot.imshow). So that makes me inclined to change the name --synthesis_status_animateor somethign seems too long to me. I could go back tosynthesis_animate. Any other ideas?versionaddedtags in the docstring of each of the consolidated functions above, but the way they are interacted with has changed somewhat. Should I try to explain that more explicitly? Or just rely on people reading the docstrings? (The section in the "Migration Guide" also basically just says "check their docstrings")Describe changes
po.plot.histogramfunction, used by thepo.plot.synthesis_histogramfunction.po.plot.histogram. Updates section on plotting functions.test_uploaded_files/ fetch capability:example_mad.pt,example_mad-cuda.pt,example_eigendistortion_color.ptpo.plot.imshowandpo.plot.animshowraise some more informative error messages around dimensionality/shape.test_eigendistortionintotest_display.Synthesis,OptimizedSynthesisto_Synthesis,_OptimizedSynthesissince they're private.Checklist
Affirm that you have done the following:
docs/, or (for large changes) adding new files to thedocs/folder.rstfiles indocs/api/or adding a new file as necessary.