Decouple dataset from plots #332
Conversation
| ) | ||
|
|
||
| pass | ||
| # def plot_imputed_values(self): # TODO not used |
There was a problem hiding this comment.
This is a requested feature and should be an issue to add the interface.
There was a problem hiding this comment.
| # method = None | ||
|
|
||
|
|
||
| class PlotUtils: | ||
| @staticmethod | ||
| def _update_colors_plotly(fig, color_dict): | ||
| # plotly doesnt allow to assign color to certain group |
There was a problem hiding this comment.
Oh yes it does. Mark as todo.
| group2, | ||
| column=None, | ||
| method=None, | ||
| *, |
There was a problem hiding this comment.
it makes people using this class downstream always use keyword arguments when creating an instance in this case
so before it was possible to have a
VolcanoPlot(mat, rawinput, ...)now it makes us explicitly do
VolcanoPlot(mat=mat, rawinput=rawinput, ...)There was a problem hiding this comment.
in terms of why do we need this — probably to prevent errors with the order of positional arguments
| if plot: | ||
| self._perform_differential_expression_analysis() | ||
| self._annotate_result_df() | ||
| self._add_hover_data_columns() | ||
| self._plot() | ||
|
|
||
| # TODO this changes the actual metadata .. is this intended? | ||
| def _add_metadata_column(self, group1_list: list, group2_list: list): | ||
| # TODO this used to change the actual metadata .. was this intended? |
There was a problem hiding this comment.
Do I get this right:
It used to add the _comparison_column to the DataSet metadata.
Now it returns the altered metadata to VolcanoPlot, which serves the purporse during the Volcano plot generation, but it is never back propagated to the metadata attribute of DataSet?
If that is so, then this is now fine.
There was a problem hiding this comment.
exactly. It overwrites the instance variable self.metadata with the updated metadata, but does not change the original value.
Note that these instance variables (mat, metadata, ...) in principle still can be changed in case they are mutable pd.DataFrame, dict, ..:
if in Statistics something like self.mat["new_column"] = 123" is done, then it will change the actual matobject of theDataSet(this fact is used to updatepreprocessing_info`)
There was a problem hiding this comment.
I am not sure I like the clutter of DataSet with all the plotting wrappers, especially considering all the feature requests that we have. Could we maybe do the same thing as with the statistics and preprocessing and initialize a plot class once with the data, so we can then do DataSet.plot.Volcano?
| @@ -17,6 +17,7 @@ | |||
| class Statistics: | |||
| def __init__( | |||
| self, | |||
| *, | |||
There was a problem hiding this comment.
is the keyword-only the new default for the repo?
There was a problem hiding this comment.
not really, it's just for these very long argument lists .. ideally, only the data that is required for the operations is passed via constructor, and parameters (e.g. for plotting) are passed in the actual plot functions..
| group2, | ||
| column=None, | ||
| method=None, | ||
| *, |
There was a problem hiding this comment.
it makes people using this class downstream always use keyword arguments when creating an instance in this case
so before it was possible to have a
VolcanoPlot(mat, rawinput, ...)now it makes us explicitly do
VolcanoPlot(mat=mat, rawinput=rawinput, ...)| group2, | ||
| column=None, | ||
| method=None, | ||
| *, |
There was a problem hiding this comment.
in terms of why do we need this — probably to prevent errors with the order of positional arguments
Again, a lot of changes at once. But mostly moved code, and changes to
__init__methods .. see individual commit, I hope they make sense