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

Rethink plotting code #234

Open
billbrod opened this issue Jan 12, 2024 · 0 comments
Open

Rethink plotting code #234

billbrod opened this issue Jan 12, 2024 · 0 comments

Comments

@billbrod
Copy link
Collaborator

Following discussion with Eric and Edoardo on #225 , should rethink how to handle plotting. Copying relevant comments here:

@EricThomson:

representation_for_plotting() it looks like could be documented more maybe, or maybe it's just something meant to be hidden away and changes a lot -- it does clearly say it's there to be used by plot_representation() as a helper function so it's probably ok that it is sort of an anomaly in terms of documentation 😆

@BalzaniEdoardo:

[high-level suggestion] Moving the visualization elsewhere. I think that having a method instead of a utility function call is not enough to justify all those lines of code for plotting in a class that that performs computation.

General comment about these plotting methods. Could they be moved out of the class? everything else is super clean and it is implementing the calculations for the model. in a way plotting seems out of scope, and the plotting is a lot of lines.

How specific is the plotting to the model structure? at a first glance, you need the scales and the necessary dict structure self._necessary_stats_dict, if you are able to provide the tensor and a metadata that is sufficient to convert to dict to a plotting function you should be able to have an external utility function that plots given the output tensor and the metadata.

me:

yeah, I don't love how I'm handling plotting right now. the idea is that plotting for every model will be different and the way to combine from the complete representation into a format for plotting is model-specific and not obvious. This gets called in the synthesis plotting code (which you can see in the PortillaSimoncelli notebook or the Metamer notebook): we just call model.plot_representation() and, if that method doesn't exist, use some default (single stem plot if the representation looks like a vector, image plot if it looks like an image). I didn't want to try and get more any clever.

But I'm not super happy with how I handle plotting, because it is messy, like you said. I'd be happy to talk about this more. But this might end up being a separate PR.

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

No branches or pull requests

1 participant