Patch/peak boundary int

[singjc]

Changed the peak boundaries vbar max int value to use the max method in the TransitionGroup object. This ensures the peak boundaries are of reasonable height based on the max intensity within the boundary to avoid really high peak vbars.

Contents (#100)

Fixes

• Remove outside addition of peaks

Other

• convet add peak boudnary menthod to private
• peak boudnary plotting
• peak boundary plotting in manuscript plots
• update plotting doc
• add feature legend labels to main plto call

Uncategorised!

• [ADD] ms level str attribute
• [UPDATE] peak boundary max int using tg max
• [MINOR] lint, add new line in method signature
• [ADD] trGrp to class plot methods
• [UPDATE] tutorials with TrGrp for add peak boundary
• [UPDATE] manuscript figures with TrGrp for add peak boundary

[singjc]

@jcharkow I propagated changes to the class plotting methods, the tutorials and the manuscript figures. Let me know if I missed any other places

[singjc]

Sorry just looking at this again I'm not a huge fan of adding the trasitionGroup as an argument because could lead to breaking functionality.

E.g. what if a different transition group is provided than the one that is plotted?

I think a better implementation might be to create a dummy transitionGroup from the current plotting object and then do the check.

OR (possibly easier)

make add_peak_boundaries a private method that is invoked by the plot() function. If a transition list is supplied then the boundaries will be plotted.

Let me know your thoughts

@jcharkow, good point. I think we can just make add_peak_boundaries a private method. The InteractivePlotter class should handle whether to plot the boundaries or not when features are provided to plot.plot_chromatogram(transitionGroup, features).

I'm not sure why TransitionGroup::plot does this separately outside the interactive plotter?

[jcharkow]

Sorry just looking at this again I'm not a huge fan of adding the trasitionGroup as an argument because could lead to breaking functionality. E.g. what if a different transition group is provided than the one that is plotted? I think a better implementation might be to create a dummy transitionGroup from the current plotting object and then do the check. OR (possibly easier) make add_peak_boundaries a private method that is invoked by the plot() function. If a transition list is supplied then the boundaries will be plotted. Let me know your thoughts

@jcharkow, good point. I think we can just make add_peak_boundaries a private method. The InteractivePlotter class should handle whether to plot the boundaries or not when features are provided to plot.plot_chromatogram(transitionGroup, features).

I'm not sure why TransitionGroup::plot does this separately outside the interactive plotter?

The idea behind the plot() function was to allow for a light wrapper for quick plotting with less customizability but without having to create a plotting object.

[jcharkow]

did you want to work on this or should I?

[singjc]

Sorry just looking at this again I'm not a huge fan of adding the trasitionGroup as an argument because could lead to breaking functionality. E.g. what if a different transition group is provided than the one that is plotted? I think a better implementation might be to create a dummy transitionGroup from the current plotting object and then do the check. OR (possibly easier) make add_peak_boundaries a private method that is invoked by the plot() function. If a transition list is supplied then the boundaries will be plotted. Let me know your thoughts

@jcharkow, good point. I think we can just make add_peak_boundaries a private method. The InteractivePlotter class should handle whether to plot the boundaries or not when features are provided to plot.plot_chromatogram(transitionGroup, features). I'm not sure why TransitionGroup::plot does this separately outside the interactive plotter?

The idea behind the plot() function was to allow for a light wrapper for quick plotting with less customizability but without having to create a plotting object.

This is fine and makes sense, but I don't see why it's necessary to call add_peak_boundaries outside of the plotter.plot call, instead of just directly passing the features object to the plotter.plot call to generate peak boundaries if features is not none.

[singjc]

did you want to work on this or should I?

I just made some changes based on making add_peak_boundaries private (__add_peak_boundaries) so that only the interactive plotter class directly deals with plotting the boundaries if a feature object is passed.

Feel free to change if there's a better option of dealing with this

[jcharkow]

Sorry misunderstanding

Sorry just looking at this again I'm not a huge fan of adding the trasitionGroup as an argument because could lead to breaking functionality. E.g. what if a different transition group is provided than the one that is plotted? I think a better implementation might be to create a dummy transitionGroup from the current plotting object and then do the check. OR (possibly easier) make add_peak_boundaries a private method that is invoked by the plot() function. If a transition list is supplied then the boundaries will be plotted. Let me know your thoughts

@jcharkow, good point. I think we can just make add_peak_boundaries a private method. The InteractivePlotter class should handle whether to plot the boundaries or not when features are provided to plot.plot_chromatogram(transitionGroup, features). I'm not sure why TransitionGroup::plot does this separately outside the interactive plotter?

The idea behind the plot() function was to allow for a light wrapper for quick plotting with less customizability but without having to create a plotting object.

This is fine and makes sense, but I don't see why it's necessary to call add_peak_boundaries outside of the plotter.plot call, instead of just directly passing the features object to the plotter.plot call to generate peak boundaries if features is not none.

Sorry misunderstanding yes it does not make sense for add_peak_boundaries outside of plot call. I must have missed something in the implementation

[singjc]

@jcharkow feel free to merge when ready