Spectrum Plots#16
Conversation
- useful if each item to plot gets a separate trace in figure (e.g. peaks in a spectrum)
- colors based on group by - custom colors for individual peaks
mz ion annotations (with colors) sequence custom annotation select top n intensity peaks for annotation or all
- includes major changes to scatter plot, now with heatmap functionality - will be useful for feature heatmap, peak map, ...
axelwalter
requested review from
jcharkow and
singjc
and removed request for
singjc
I'm not too sure I understand what is needed? Do we need to implement a different kind of heatmap plot?
I personally would prefer to leave it as x and y for consistency with the other kinds of plots, when using the I was looking into including a higher-level plot method for each specific kind, i.e. See: https://pandas.pydata.org/pandas-docs/stable/development/extending.html
I can try look into this. Did you use the new app-devel-spectrum.py to test for this?
Do we need to draw the grid lines before drawing the scatter points?
The base plot config is used to update all the defaults if specific parameters (i.e. xlabel) is not passed directly to the So if What we could do, is add a mapping for different axis labels that correspond to their kind of plot, and then we can pass kind to the _BasePlotConfig to configure the default values for axis-labels and other params that of plot specific. |
singjc
left a comment
singjc
left a comment
There was a problem hiding this comment.
I briefly skimmed, but everything looks fine to me. I was wondering about two things though:
-
When a Spectrum plot is requested, and ion_mobility is set, then it generates an ion mobility vs m/z PeakMap. I think this can already be done by using the feature_heatmap (which we can rename to PeakMap) kind of plot instead? I just think it might be confusing, if you're asking for a spectrum plot, but provide an ion mobility column to generate a PeakMap plot?
-
For the scatter plot marker cycler, if we use
byto plot by a grouped df, I'm not sure how this will look for a RT vs IM peak map, but I think it might look weird with all the different markers? The RT vs IM peak map is more dense then a MZ vs IM peak map, so all the different markers might make the RT vs IM peak map hard to visualize.
There is a README in the test/test_data that is still in the pyopenms_viz folder, I don't think it got moved with the rest of test/test_data to the main project folder for some reason. Can you add some info in that README about the two new test tsv files, just so we know how they were generated / where they come from.
@jcharkow may have more suggestions / comments.
pyopenms_viz/_core.py
Outdated
| mz: str, | ||
| intensity: str, |
There was a problem hiding this comment.
I would prefer to keep these as x and y
There was a problem hiding this comment.
Yes, that's what I would prefer too. Just did that for the ion mobility option to make things clear. Acutally I really like the suggestion to rename the FeatureHeatmapPlot to a generic PeakMap which can plot ion mobility spectra, MS experiments, feature heatmaps etc. Then we could keep it simply with x and y and the PeakMap would have an optional "intensity".
There was a problem hiding this comment.
Yes I would keep "x" and "y" too, For 3D maps I would label "z" instead of intensity as well
pyopenms_viz/_core.py
Outdated
| if self.ion_mobility is None: | ||
| self.plot(spectrum, reference_spectrum, mz, intensity, **kwargs) | ||
| else: | ||
| self.plot_ion_mobility(spectrum, mz, ion_mobility, intensity, **kwargs) |
There was a problem hiding this comment.
Couldn't this be done with the FeatureHeatmap (which we can rename to PeakMap) Plot? i.e. df.plot(kind='feature_heatmap', x='mz', y='im')?
There was a problem hiding this comment.
The reason for implementing it in scatter was to plot universal peak maps with scatter. But renaming the FeatureHeatMap to PeakMap would be very nice and clear.
| def plot(self, x, y, **kwargs): | ||
| def plot(self, spectrum, reference_spectrum, x, y, **kwargs): | ||
| """Standard spectrum plot with m/z on x-axis, intensity on y-axis and optional mirror spectrum.""" | ||
| kwargs.pop("fig", None) # remove figure from **kwargs if exists |
There was a problem hiding this comment.
Passing the spectrum and reference_spectrum? I modify them with the prepare_data method (relative intensities etc). before calling either the normal spectrum plot method or the ion mobility spectrum plot.
Yes, best solution!
Hm it is optional and in such a case the user doesn't need to use "by"? It looks nice for scatter plots which are not too crowded. I get your concern but would leave that to the user to chose the right settings which look good. |
The question was how can the user modify the markers in the current setup when we create a marker_dict for the heat map it is kind of pre-defined. One way would be to update the marker_dict with values from a custom passed marker_dict. But this is probably not very important in real use cases.
True, and if we plot ion mobility spectra as peak maps that will be perfectly fine.
This would be nice to have but the current method by chosing the kind of plot seems simple enough. I would set this as a potential future feature if on time constraints right now.
Thanks! Yes, tested everything with the
Would need to remove the order, should work yes. First update plot aes and then plot.
If we have a generic PeakMap Plot which will be used to plot feature heatmaps, ion mobility spectra and experiments we can set sane defaults right away and don't need to change that (like now with spectrum and ion mobility spec). |
Yes I agree with changing FeatureHeatMap to PeakMap. Will review the rest of the code shortly |
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
I have not tested it but looks good. Just a few minor suggestions. I like the idea of combining FeatureHeatMap into a PeakMap object and am excited to see those changes.
- can be used to plot feature heatmaps, MS experiment 2D peak maps, ion mobility spectra
similar to ColorGenerator returns a generator with marker shapes based on the plotting engine
determine peak color in _get_colors only, not in _get_annotations bokeh peak color
sort z values to plot highest on top relative intensity peak binning example notebook
Great, thanks for making the changes! I think that sounds good and makes sense 👍. We can probably include a link to the OpenMS web app template repo in the README to redirect/showcase using the plotting framework in a web app? |
Sure sounds great! Will add a pyopenms-viz page to the template app and link here in the README once this repo is public / released. |
|
One thing missing here which I will add eventually is the 3D peak map with matplotlib. |
Oh right, I think you did have code to do this in the MSExperimentPlotter in the initial version? Do you think we could re-use some of that code? Do we need to add another base class for 3D peak maps? |
Yes the code is in the initial version. We could adopt the VLine Plot to color lines with intensity values. However, the figure itself needs to be initialized differently as a 3D figure. |
3 participants
Added the full functionality for spectrum plots.
Due to time constraints there are no example notebooks yet (will be in a separate PR). However, I included a temporary streamlit app file showcasing everything (app-devel-spectrum.py).
Some questions / discussion points remain: