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

Can only specify a visaulization once #34

Open
samwalkow opened this issue Aug 31, 2022 · 1 comment
Open

Can only specify a visaulization once #34

samwalkow opened this issue Aug 31, 2022 · 1 comment
Labels
enhancement New feature or request

Comments

@samwalkow
Copy link
Member

samwalkow commented Aug 31, 2022

Description

Once you pick a visualization, that option disappears. What if you want to create a list of two projection plots? That's not possible right now.

What I Did

In the schema_model.py file, this is how the Plot attribute is set up. How can we reuse specifications?

 Plot: Optional[List[Visualizations]]

I think we'll have to change the Visaulizations dataclass. Maybe we make more lists?

Right now we have:

class Visualizations(ytBaseModel):
    """
    This class organizes the attributes below so users
    can select the plot by name,
    and see the correct arguments as suggestions
    """

    SlicePlot: Optional[SlicePlot]
    ProjectionPlot: Optional[ProjectionPlot]
    PhasePlot: Optional[PhasePlot]

Should it be:

    SlicePlot: Optional[List[SlicePlot]]
    ProjectionPlot: Optional[List[ProjectionPlot]]
    PhasePlot: Optional[List[PhasePlot]]

@samwalkow samwalkow added the enhancement New feature or request label Aug 31, 2022
@chrishavlin
Copy link
Collaborator

I think your suggestion of putting the *Plots into lists would work. If we do that, how do you feel about renaming the Visualizations attributes to be plural? i.e.,:

class Visualizations(ytBaseModel):
    """
    This class organizes the attributes below so users
    can select the plot by name,
    and see the correct arguments as suggestions
    """

    SlicePlots: Optional[List[SlicePlot]]
    ProjectionPlots: Optional[List[ProjectionPlot]]
    PhasePlots: Optional[List[PhasePlot]]

IMO it's a nice way to grammatically signal that you can put multiples of any of them.

The new Visualizations runner in _model_instantiation would need some minor tweaking to account for the lists. See here:

def process_pydantic(self, pydantic_instance: data_classes.Visualizations, ds=None):
generic_runner = YTGeneric()
viz_results = {}
for attr in pydantic_instance.__fields__.keys():
viz_model = getattr(pydantic_instance, attr) # SlicePlot, etc.
if viz_model is not None:
result = generic_runner.run(viz_model, ds=ds)
nme = f"{ds.basename}_{attr}"
viz_results[nme] = self._sanitize_viz(viz_model, result)
return viz_results

@chrishavlin chrishavlin added this to the initial release milestone Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants