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

Add annotations to plotly plots #1321

Merged
merged 6 commits into from
Feb 11, 2025
Merged

Add annotations to plotly plots #1321

merged 6 commits into from
Feb 11, 2025

Conversation

asizemore
Copy link
Member

Resolves #1315

Uses plotly's layout.annotations prop to add annotations to plots (see examples). Examples included for Scatterplot and Histogram, but these annotations could be added to any plotly plot.

This PR does not add annotations for visx plots. I considered that as out of scope for the time being.

@asizemore asizemore requested a review from dmfalke February 5, 2025 15:56
Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

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

This looks great! I do wonder if we should consider hiding plotly internals a bit more. It would be nice to have a generic interface for annotations that we can use with all plots. Is this a reasonable thought? Do we have other examples plots with annotations?

@asizemore
Copy link
Member Author

Ahh good point. While actually implementing the visx annotations might be out of scope, you're right that we should at least try to define a nice neutral interface that they could both use. I'll work on this!

@dmfalke
Copy link
Member

dmfalke commented Feb 10, 2025

Ahh good point. While actually implementing the visx annotations might be out of scope, you're right that we should at least try to define a nice neutral interface that they could both use. I'll work on this!

Cool! Let me know if you want to bounce ideas around. Something I'm wondering is, what is a reasonable approach to declaring where an annotation will appear. for example, are the annotations for the crispr scatterplots based on min and max points? more generally, is this specific to the dataset type? do we need a crispr-specific scatterplot component to declare these annotations?

Anyway, just some random thoughts about this.

@asizemore
Copy link
Member Author

Thanks @dmfalke ! I just added a new type. Plotly and visx approach annotations pretty differently, so I attempted to keep the type minimal and let it expand in the future if we need.

Something I'm wondering is, what is a reasonable approach to declaring where an annotation will appear. for example, are the annotations for the crispr scatterplots based on min and max points?

There are two ways to define where an annotation goes (in plotly): using a fraction of the axis distance (plotly's 'paper' reference') or an actual point on the plot. The stories I put up have an example of each. But i think your question about a crispr-specific plot is a good one. We don't necessarily need one because we could have another attribute in john's table that declares the annotation position as long as that position is with reference to the axes, not xmin and max. I believe that is the case for these plots - folks just want to see those annotations in the bottom left and top right, but I need to verify.

So we don't have to (at least not from what i see right now!) but of course we could!

@asizemore
Copy link
Member Author

To answer your earlier question, we do have a plot that uses annotations right now: the volcano visx plot. It has volcano-specific annotations already baked in, and the only thing it needs to know from the context is the text for those annotations. Our thinking was that all volcano plot should always have those annotations, so no need to worry about passing in more than just the annotation text (called the comparisonLabels prop).

@dmfalke
Copy link
Member

dmfalke commented Feb 11, 2025

@asizemore overall, I think this looks great. I'm not sure if you saw this build error: https://github.com/VEuPathDB/web-monorepo/actions/runs/13252170333/job/36992249053?pr=1321.

@dmfalke
Copy link
Member

dmfalke commented Feb 11, 2025

Once that build error is fixed, I think this can be merged.

@asizemore
Copy link
Member Author

oh i didn't! Glad you pointed it out!

Sounds good! I'll fix and merge!

@asizemore asizemore merged commit ae6bc00 into main Feb 11, 2025
1 check passed
@asizemore asizemore deleted the feature-1315-annotations branch February 11, 2025 21:31
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

Successfully merging this pull request may close these issues.

Phenotype data: Add annotations to scatterplot component
2 participants