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 annotation labels for 3D plots #41

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

t0mdavid-m
Copy link
Member

This PR adds annotation labels for 3D plots. It can be tested easily using the PeakMap example jupyter notebook with the following sample DataFrame:

annotation_data = pd.DataFrame(
    {
        'leftWidth' : [212],
        'rightWidth' : [237],
        'IM_leftWidth' : [266],
        'IM_rightWidth' : [272],
        'color' : ['blue'],
        'name' : ['test'],
    }
)

While I was at it I also enabled the colors for the 2D visualizations (optional). I noticed that the rectangular 2D annotation was shown behind the plot data and made sure that it is displayed in the foreground.

I also noticed that annotation color and label for 3D plots is set using the input DataFrame. I propose moving these annotations to the annotation_data DataFrame to simplify the user experience.

Copy link
Collaborator

@jcharkow jcharkow left a comment

Choose a reason for hiding this comment

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

Looks good just some minor suggestions. @singjc is more familiar with this so I would wait for his input before merging. I have not tested this out so assuming it does not break the existing 2D annotations.

pyopenms_viz/_core.py Outdated Show resolved Hide resolved
@@ -732,7 +757,6 @@ def _add_box_boundaries(self, annotation_data):
loc=matplotlibLegendLoc,
title=self.feature_config.legend.title,
prop={"size": self.feature_config.legend.fontsize},
bbox_to_anchor=self.feature_config.legend.bbox_to_anchor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this line removed? I think this is needed for 2D legend placement

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry - I forgot to mention this. There seems to be a weird interaction between bbox_to_anchor and tight_layout(), where the plot gets resized when the legend is added. With the legend enabled not only the plot area shrinks (expected as the legend needs space) but also the entire figure gets resized in a strange way (unexpected).

If I disable tight_layout() in MATPLOTLIBPlot.show_default() or remove the bbox_to_anchor parameter the issues disappear.

How would you recommend to handle this issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you disable tight_layout in MATPLOTLIBPlot.show_default() and keep bbox_to_anchor, after the plot get's generated, if you call plt.tight_layout(), does it still cause strange resizing?

Copy link
Member Author

Choose a reason for hiding this comment

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

No this did not resolve this issue on its own. Calling plt.tight_layout() after the fact seemed to not have any effect. Adjusting the anchoring of the bbox seems to result in nice plots (and should be more stable from what I can tell):
image

),
showlegend=True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should legend showing be configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we could make it optional for 3D plots as we have the in-plot annotation labels with this PR. For 2D-plots, the legend is currently the only way to pick a location so I am not sure how useful that would be.

We could add the same functionality but I am unsure how to best choose a good location for the in-plot annotation labels. Maybe we could allow for additional (x, y)-label-locations in annotation_data and let the user decide? What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this for adding in-plot text annotations 2D plots, that requires creating a dummy legend?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any updates on this? I am not too familiar with Plotly so not sure what is best

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this for adding in-plot text annotations 2D plots, that requires creating a dummy legend?

A dummy legend is not necessary for in-plot annotations afaik. I was just wondering how useful making the legend optional would be if there is no alternative. Thus, I suggested adding annotation labels to 2D-Plots as well but am unsure about a good general placement for the label.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we could require a set of coordinates for the user. This could be an optional feature for the 3D-Plots as well.

Copy link
Collaborator

@singjc singjc left a comment

Choose a reason for hiding this comment

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

I tested and updated the manuscript_supp_config_gallery with the text annotations. It seems to work nicely with matplotlib and plotly. I did add a commit to allow adjusting the font size of the text annotations.

Other than Josh's comments/suggestions, I think everything looks good.

@jcharkow
Copy link
Collaborator

jcharkow commented Oct 8, 2024

Hopefully we can merge soon?

@singjc
Copy link
Collaborator

singjc commented Oct 11, 2024

Hopefully we can merge soon?

I think it is ready to merge.

@t0mdavid-m
Copy link
Member Author

I agree @singjc .

Did you also see my reply to the comment regarding the configurable legend? @jcharkow @singjc

If we decide to implement it I suppose it could also be done in a separate PR.

@singjc
Copy link
Collaborator

singjc commented Oct 14, 2024

I agree @singjc .

Did you also see my reply to the comment regarding the configurable legend? @jcharkow @singjc

If we decide to implement it I suppose it could also be done in a separate PR.

I think your suggestion for having additional x-y coordinate locations for positioning the labels could be an option? Then the legend could be set as configurable to display a legend or not. We could add it to this PR if it's not too much/won't take too long, or we could implement in another PR.

I think we would like to merge this PR relatively soon, since it is used in the manuscript.

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.

3 participants