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

feat[frontend]: implementation and testing for VisualizationSpec #2147

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

groberts-flex
Copy link
Contributor

Adding ability to have a visualization specification for a medium that overrides the plotting parameters in the structure plotting code so that structures in the gui can be specified to have fixed colors and not change when new structures are added. For task "VisualizationSpec for Structure / Medium".

Attached below are screenshots from local testing to show the ability to set different face color, edge color, and alpha as well as usage of default plotting parameters when no visualization specification is included in the medium

Magenta, Cyan, 0.1
magenta_cyan_alpha_p1
Red, Blue, 0.5
red_blue_alpha_p5
No visualization spec included in medium
no_viz_spec

@groberts-flex
Copy link
Contributor Author

multiple structures with no viz specs
multi_structure_no_viz_specs

multiple structures with multiple viz specs
multi_structure_with_viz_specs

Right now, the edge colors for the structures do not show up because the scene code used for plotting a simulation is plotting with linewidth=0. I believe it is straightforward to keep extending viz spec to include things like linewidth or other plotting parameters. The current behavior is if the viz spec is present in a medium, it overrides just the plot_params properties that it contains.

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Nice thanks @groberts-flex, this is already looking really good. Most of the comments are pretty minor, so this should be good to go after a little bit of iteration!

Copy link

Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Thanks @groberts-flex , this is high quality work and I think the feature will be very useful and timely for us. I had a couple high level comments but will leave the rest to yannick and dario.

  1. could you add a short changelog item to describe the feature?
  2. this PR introduces some more coupling between tidy3d/components and tidy3d/viz.py. Namely the VizSpec is defined in viz.py rather than in components. We are working towards making matplotlib an optional dependency of tidy3d to make it a more lightweight package. In this PR for example we are doing some changes to how viz are done for this goal. I think in the long run we want to also consider plotting as a "wrapper" around the core pydantic model definition. I think this PR accomplishes that more or less because of the PlotParams.override_with_viz_spec method. My question (largely for @yaugenst-flex too perhaps) is whether any of this architecture conflicts with our long term plan to decouple plotting from the data structure definition? and if so, do we want to for example move VizSpec into components?

@groberts-flex
Copy link
Contributor Author

Thanks @tylerflex for the comments! There is some explicit coupling to matplotlib in the VisualizationSpec class as it uses a function from matplotlib in a validator to ensure the user has specified a recognizable color for plotting. However, I'm sure this can be decoupled especially if there is a standard set of allowed colors to check the user input against. There are likely bigger considerations than just this, however, and I'll defer for now to @yaugenst-flex on the fuller answer to the question.

@yaugenst-flex yaugenst-flex self-requested a review January 8, 2025 19:03
@yaugenst-flex yaugenst-flex added the 2.8 will go into version 2.8.* label Jan 8, 2025
Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Ok nice this is looking good! One last thing I just noticed.. this should be merged into pre/2.8 and not into develop. The plotting import in viz.py will need to be lazy then (you'll see). So I think maybe address those two minor comments I left and then apply the changes from your commits to the pre/2.8 branch instead.

@yaugenst-flex
Copy link
Collaborator

yaugenst-flex commented Jan 8, 2025

@tylerflex @groberts-flex regarding where VisualizationSpec should live: Yeah this is a bit tricky, actually @groberts-flex had this in a separate component initially. The problem with that is I don't think we should have a separate module just for this change, and it also doesn't really feel like this belongs in medium.py, especially if we end up adding this to Structure too. In that sense I think viz.py is the correct place. As long as the import is done lazily (will happen when this is rebased to pre/2.8), then this doesn't conflict at least with the current state of the minimal install.

In terms of long-term restructuring of visualization / repo I think this change is net neutral because this all needs to be revisited anyways (also e.g. for #2141).
So in my opinion this is fine as-is for now.

Maybe @daquinteroflex has additional thoughts?

@groberts-flex groberts-flex force-pushed the groberts-flex/vizspec-struct-medium branch from eb1e9ed to ba3ccf4 Compare January 9, 2025 03:22
@groberts-flex groberts-flex changed the base branch from develop to pre/2.8 January 9, 2025 03:23
@yaugenst-flex
Copy link
Collaborator

yaugenst-flex commented Jan 9, 2025

There is some explicit coupling to matplotlib in the VisualizationSpec class as it uses a function from matplotlib in a validator to ensure the user has specified a recognizable color for plotting.

Regarding this point, yeah this might be a bit of a problem. The idea behind the minimal install is to be able to validate simulations in a very constrained environment, ideally without matplotlib as a dependency.
We could either:

  1. Skip the color validator if matplotlib is not importable.
  2. Implement the color validation ourselves.
  3. Leave it as-is.

Option 1 is probably not great because it can lead to GUI bugs.
Option 2 seems like a pain, but actually solves the problem.
Option 3 might be ok too because the way that validation works (as far as I understand) is that if it fails in the minimal environment, it will be run on a full environment, which will still work. And since the viz_spec is None by default, the validation should work in a minimal environment in most cases.

I would be ok with both 2 or 3 for now, not sure how involved option 2 is.

@daquinteroflex
Copy link
Collaborator

Nice work Greg, look forward to more of your contributions! VizualizationSpec as defined looks reasonable and should interface nicely with the GUI and yeah viz.py makes sense for now. Maybe in the future it'll be viz/ after the refactor.

So yeah, the goal is to restructure the way we do plotting in the future so it is more "easier to extend", completely on-the-fly, clear on what's actually happening to the figure (easier to debug), and have it self contained/decoupled from the internal class parameters themselves as possible. In this sense, some of these methods may need to be restructured in the future. (Your help would be most welcome if you're interested eventually!)

So now that we can merge #2087, this PR rebase onto that might identify some changes on the imports as Yannick and Tyler suggest. Re the validator, option 3 looks good if it can be tested too (we would really benefit from testing it eventually in this #2073 but that'd be down the line.

@groberts-flex groberts-flex force-pushed the groberts-flex/vizspec-struct-medium branch from ba3ccf4 to aaae37a Compare January 9, 2025 19:22
@groberts-flex
Copy link
Contributor Author

Thanks Dario, that all sounds reasonable to me, thanks for the details on where viz stuff is headed! I rebased the changes and settled the merge conflicts in viz.py

@prashkh
Copy link
Contributor

prashkh commented Jan 9, 2025

@groberts-flex Thanks for implementing this feature so quickly. The way it is implemented at the medium level works great for implementation in PhotonForge. We don't have to change much! Thanks again.

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

I'll approve this now, as far as I can see it's ready to go. We might want to add some additional plotting parameters to VisualizationSpec down the line though.

Only one tiny thing: Could you change the commit message @groberts-flex to explicitly mention the addition of VisualizationSpec?

@groberts-flex groberts-flex force-pushed the groberts-flex/vizspec-struct-medium branch from aaae37a to 90fd369 Compare January 10, 2025 12:06
@yaugenst-flex yaugenst-flex merged commit 77d4bbb into pre/2.8 Jan 10, 2025
15 checks passed
@yaugenst-flex yaugenst-flex deleted the groberts-flex/vizspec-struct-medium branch January 10, 2025 15:21
@e-g-melo
Copy link
Collaborator

Thanks @groberts-flex! It is a very nice new feature!

From the GUI side, it seems nice. I have no additional comments.

In GUI, users pick up colors from this interface. So, it should work very well.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 will go into version 2.8.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants