Skip to content

MSSpectrum plots#3

Merged
axelwalter merged 21 commits intoOpenMS:mainfrom
axelwalter:main
Jun 10, 2024
Merged

MSSpectrum plots#3
axelwalter merged 21 commits intoOpenMS:mainfrom
axelwalter:main

Conversation

@axelwalter
Copy link
Collaborator

@axelwalter axelwalter commented May 15, 2024

No description provided.

@axelwalter axelwalter requested a review from jcharkow June 3, 2024 13:03
- input is pyopenms.MSExperiment().get_df(long=True)
- relative intensity support
- can bin peaks to reduce complexity and increase performance
- binning done with pandas internally, simpler and less dependencies then datashader and holoviews
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.

Great work! Had a few minor comments with the main one being possibly we should make the ion mobility source code in a separate function so it is easier to locate.
I think some of this code is overlapping with the chromatogram plotting interface so we will need to do some work with integrating them upon merging.

Also please include test files so that I can run it on my end.

Comment on lines +13 to +23
# A colorset suitable for color blindness
class Colors(str, Enum):
BLUE = "#4575B4"
RED = "#D73027"
LIGHTBLUE = "#91BFDB"
ORANGE = "#FC8D59"
PURPLE = "#7B2C65"
YELLOW = "#FCCF53"
DARKGRAY = "#555555"
LIGHTGRAY = "#BBBBBB"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would rather allow users to specify a palette rather than hardcode colors. We have an implementation for this with the chromatogram plotter can adapt to here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since each peak is represented by a row in the dataframe, users can specify extra columns for custom peak color, custom annotation color and custom annotation text. This will override this default coloring. What do you think about that solution?

Just annotating mz with default colors:
image

Setting custom peak (all green) and annotation colors (all blue) defined in df:
image

Annotating ions default colors:
image

Annotating ions AND custom color (all blue) includes peak and annotation text:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that!

def engine_enum(self):
return Engine[self.engine]


Copy link
Collaborator

Choose a reason for hiding this comment

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

remove extra line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we use the same formatter? Using black results in these double empty lines after class definition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm not quite sure what you mean

Comment on lines +123 to +131
ax.set_xlabel(self.config.xlabel, fontsize=10, color=Colors["DARKGRAY"])
ax.set_ylabel(
self.config.ylabel, fontsize=10, style="italic", color=Colors["DARKGRAY"]
)
ax.xaxis.label.set_color(Colors["DARKGRAY"])
ax.tick_params(axis="x", colors=Colors["DARKGRAY"])
ax.yaxis.label.set_color(Colors["DARKGRAY"])
ax.tick_params(axis="y", colors=Colors["DARKGRAY"])
ax.spines[["right", "top"]].set_visible(False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if hardcoding colors is the best idea

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just for axis and label text colors. Should that be customizable by the user? Where to stop (font sizes, font weight, padding, tickformatters, etc...) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we are changing the defaults then we should have a global variable somewhere for different plot settings (e.g. jupyter notebook vs streamlit) does not necessarily have to be accessible to user. It will also be easier to change later if we want to change defaults

Comment on lines +54 to +62
colormap = {
"a": Colors["PURPLE"],
"b": Colors["BLUE"],
"c": Colors["LIGHTBLUE"],
"x": Colors["YELLOW"],
"y": Colors["RED"],
"z": Colors["ORANGE"],
}
return colormap.get(annotation.lower(), Colors["DARKGRAY"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO integrate this into common color framework

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How would that look like? We can discuss in person.

x=peak["mz"],
y=intensity,
text=text,
text_font_size="8pt",
Copy link
Collaborator

Choose a reason for hiding this comment

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

add this to config instead of harcoding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, there should be some universal settings defined in BasePlotter.

@axelwalter
Copy link
Collaborator Author

Thanks for the comments, I will work on them tomorrow. Agree on the ion mobility part. We will see how to resolve the overlaps. As for testing I have started working on a demo streamlit app to showcase stuff and parameters easily. Also working with the new plotly events from streamlit 1.35 to update the plot while zooming in with the MSExperiments.

- separate MSExperiment 2D and 3D plotting
- separate MSSpectrum ion mobility plotting
- Spectrum plot x-axis adjustment to not cut off peaks and annotations
- fix legend always visible in Bokeh spectrum plot
@axelwalter axelwalter requested a review from jcharkow June 6, 2024 09:23
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 to merge. Next steps is to determine parameters to be added to the base plotter however that will be easier to address when everything is merged and we can see which parameters are used across the different plotting tools.

Comment on lines +13 to +23
# A colorset suitable for color blindness
class Colors(str, Enum):
BLUE = "#4575B4"
RED = "#D73027"
LIGHTBLUE = "#91BFDB"
ORANGE = "#FC8D59"
PURPLE = "#7B2C65"
YELLOW = "#FCCF53"
DARKGRAY = "#555555"
LIGHTGRAY = "#BBBBBB"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that!

def engine_enum(self):
return Engine[self.engine]


Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm not quite sure what you mean

Comment on lines +123 to +131
ax.set_xlabel(self.config.xlabel, fontsize=10, color=Colors["DARKGRAY"])
ax.set_ylabel(
self.config.ylabel, fontsize=10, style="italic", color=Colors["DARKGRAY"]
)
ax.xaxis.label.set_color(Colors["DARKGRAY"])
ax.tick_params(axis="x", colors=Colors["DARKGRAY"])
ax.yaxis.label.set_color(Colors["DARKGRAY"])
ax.tick_params(axis="y", colors=Colors["DARKGRAY"])
ax.spines[["right", "top"]].set_visible(False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we are changing the defaults then we should have a global variable somewhere for different plot settings (e.g. jupyter notebook vs streamlit) does not necessarily have to be accessible to user. It will also be easier to change later if we want to change defaults

@axelwalter axelwalter merged commit b2591bf into OpenMS:main Jun 10, 2024
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.

2 participants