Adding Plotly Configurations #544

ronjakrg · 2024-11-15T12:10:57Z

Description

fixes #
Added constants to export workflow message
Added new plotly template for custom configuration for all plots
Included template in data preprocessing plots
Renamed color constants, changed the order to have the primary and secondary colors at index 0 and 1 - and included those changes in other files accordingly

Changes

/constants/colors.py got new color names
/constants/paths.py has small change in composition of paths
/data_analysis/model_evaluation_plots.py uses new color naming
/data_analysis/plots.py uses new color naming
/data_integration/di_plots.py uses new color naming
/data_preprocessing/plots.py no longer sets manual settings for plotly, got some refactoring to be more consistent with plotly properties and methods and uses new color naming
/utilities/plot_template.py has a new plotly template which is set as default
/ui/runs/forms/data_analysis.py fixed typo in input parameter
/ui/runs/forms/data_integration.py uses new color naming
/ui/runs/views.py has new message text

Testing

PR checklist

Development

If necessary, I have updated the documentation (README, docstrings, etc.)
If necessary, I have created / updated tests.

Mergeability

main-branch has been merged into local branch to resolve conflicts
The tests and linter have passed AFTER local merge
The code has been formatted with black

Code review

I have self-reviewed my code.
At least one other developer reviewed and approved the changes

github-actions · 2024-11-15T12:14:16Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
protzilla
run.py
run_helper.py
runner.py
protzilla/data_analysis
differential_expression_helper.py					125, 143
differential_expression_kruskal_wallis.py					227
differential_expression_mann_whitney.py					266
differential_expression_t_test.py
plots.py
protein_graphs.py
ptm_analysis.py					56, 123-125
protzilla/data_integration
di_plots.py
protzilla/data_preprocessing
outlier_detection.py
plots.py					34, 66-67, 148, 334-337
transformation.py
protzilla/importing
ms_data_import.py					122, 276
protzilla/methods
data_analysis.py					21, 174, 250, 283, 308, 351, 916, 935
data_preprocessing.py
importing.py					144
protzilla/utilities
transform_dfs.py
ui/main
settings.py
ui/runs
fields.py
views.py					21, 33, 250, 284
views_helper.py
ui/runs/forms
data_analysis.py					318-322, 379, 409, 445, 468, 552, 1183, 1221, 1243
data_integration.py
Project Total

The report is truncated to 25 files out of 58. To see the full report, please visit the workflow summary page.

_{This report was generated by python-coverage-comment-action}

gitjannes

Mir ist nichts unbeabsichtigtes aufgefallen. 👍

sarahvgls

Finde ich voll cool, dass du dich an dieses Thema rangetraut hast und auch schon in so vielen Files Änderungen gemacht hast!! Danke dafür auf jeden Fall :)
Ich hab ein paar kleinere Anmerkungen oder Fragen dran geschrieben, unter anderem würde ich gerne die import * Stellen anders behandeln, damit man dann genauer sieht, was wo in welcher File von wo verwendet wird. Kannst ja mal gerne sagen, was du dazu meinst ^^

protzilla/constants/paths.py

sarahvgls · 2024-11-21T12:16:28Z

protzilla/data_analysis/model_evaluation_plots.py

@@ -1,7 +1,7 @@
 import matplotlib.pyplot as plot
 from sklearn.metrics import PrecisionRecallDisplay, RocCurveDisplay

-from protzilla.constants.colors import PROTZILLA_DISCRETE_COLOR_SEQUENCE
+from protzilla.constants.colors import *


Ich frage mich, ob es hier vielleicht sinnvoll/"Best Parctice" wäre, die Namen der Konstanten zu importieren? Dann weiß man, was es gibt, ohne dass man die andere File anschauen muss (für Code-Verständlichkeit und so, musste an SWT denken)

Genau das gleiche habe ich mich auch gefragt ... Meine Gedanken waren hierzu:

Der Name protzilla.constants.colors sollte intuitiv genug sein, dass sich in der File halt konstante Variablen mit Farben befinden.

Nicht unbedingt in data_analysis, aber in den anderen Sections werden eigentlich alle drei Variablen (Liste, primary und secondary Farben) verwendet - daher fand ich es an der Stelle sinnvoller, einfach * zu schreiben, statt etwas umständlich alle drei einzeln zu importieren.

Ich schreibe aber mal unten als einzelnen Kommentar, was mir gerade generell als Kompromiss einfällt! :)

protzilla/data_analysis/model_evaluation_plots.py

sarahvgls · 2024-11-21T12:17:35Z

protzilla/data_analysis/plots.py

@@ -8,7 +8,7 @@
 from scipy import stats
 from sklearn.metrics.pairwise import cosine_similarity, euclidean_distances

-from protzilla.constants.colors import PROTZILLA_DISCRETE_COLOR_SEQUENCE
+from protzilla.constants.colors import *


hier würde ich das gleiche wie oben anmerken

sarahvgls · 2024-11-21T12:21:18Z

protzilla/data_analysis/plots.py

@@ -326,8 +326,8 @@ def prot_quant_plot(
    fig = go.Figure()

    color_mapping = {
-        "A": PROTZILLA_DISCRETE_COLOR_SEQUENCE[0],
-        "C": PROTZILLA_DISCRETE_COLOR_SEQUENCE[1],
+        "A": PROTZILLA_COLOR_SEQUENCE[0],


warum hast du manchmal [0] und manchmal primary color? Wäre es da sinnvoll das irgendwie auch einheitlich zu machen?

sarahvgls · 2024-11-21T12:22:20Z

protzilla/data_integration/di_plots.py

@@ -7,7 +7,7 @@
 from protzilla.constants.protzilla_logging import logger
 from protzilla.utilities.utilities import fig_to_base64

-from ..constants.colors import PROTZILLA_DISCRETE_COLOR_SEQUENCE
+from ..constants.colors import *


hier auch die gleiche Bemerkung wie oben, bzw könnte man hier halt genau nur die eine Konstante importieren, da nur die verwendet wird und in anderen Files wo mehr verwendet wird, dann alle importieren

sarahvgls · 2024-11-21T12:24:47Z

protzilla/data_preprocessing/plots.py

@@ -325,10 +270,9 @@ def create_anomaly_score_bar_plot(
    :param anomaly_df: pandas Dataframe that contains the anomaly score for each\
    sample, including outliers and on-outliers samples
    :param colour_outlier: hex code for colour depicting the outliers.
-    Default: PROTZILLA_DISCRETE_COLOR_OUTLIER_SEQUENCE outlier colour
+    Default: PROTZILLA_SECONDARY_COLOR


Verstehe ich es richtig, dass die secondary color generell die "outlier color" ist? Wenn ja, fände ich es schön, dafür einen Kommentar in der colors.py zu haben, dann weiß man das direkt?

sarahvgls · 2024-11-21T12:27:18Z

ui/runs/forms/data_analysis.py

@@ -520,7 +520,7 @@ class PlotClustergramForm(MethodForm):
        label="Choose dataframe to be used for coloring",
        required=False,
    )
-    flip_axis = CustomChoiceField(
+    flip_axes = CustomChoiceField(


(Habe hier gerade noch nicht ganz den Code Überblick, daher ist die Frage vielleicht unnötig) Ist die Änderung auch an der Stelle, wo die Variable verwendet wird, angepasst oder wo wäre das dann?

sarahvgls · 2024-11-21T12:27:47Z

ui/runs/views.py

@@ -423,7 +423,7 @@ def export_workflow(request: HttpRequest, run_name: str):
    display_message(
        {
            "level": 20,
-            "msg": f"Workflow '{requested_workflow_name}' was exported successfully.<br>You can view it here: {os.path.join(settings.BASE_DIR, 'user_data', 'workflows')}",
+            "msg": f"Workflow '{requested_workflow_name}' was exported successfully.<br>You can view the file at {WORKFLOWS_PATH}.",


Viel schöner :D

ronjakrg · 2024-11-21T20:58:43Z

Kompromiss

(Danke an @sarahvgls für das Anstoßen der Diskussion in den Reviews!)
Ich hätte folgende Idee, auch vor dem Hintergrund der Pläne von @noLeonardo : Was haltet ihr davon, wenn wir die Konstante PROTZILLA_COLOR_SEQUENCE zu PLOT_COLOR_SEQUENCE umbenennen? Denn hier möchten wir die Farben für die Plots ablegen, die nicht unbedingt gleich mit unseren PROTzilla-Farben sind müssen. (Auch weil in Zukunft der User eigene Farben in den Settings hinterlegen können soll.)
Dazu tendiere ich dazu, die einzelnen Variablen PROTZILLA_PRIMARY_COLOR und PROTZILLA_SECONDARY_COLOR wieder rauszunehmen, und die Liste stattdessen umzusortieren, sodass an Index 0 immer die primary color und an Index 1 immer die secondary color steht, die immer für die zweifarbigen Plots verwendet werden (natürlich mit entsprechendem Hinweis im Code). Das wäre insgesamt doch konsistenter als meine vorherige Idee.

Ich bin gespannt auf Eure Meinungen hierzu! :)

sarahvgls · 2024-11-22T18:39:59Z

Umbenennung zu PLOT_COLOR_SEQUENCE finde ich sinnvoll nach deinen genannten Gründen.
Ich muss sagen mir gefällt es schon, dass die Konstanten Primary color und secondary color exisitieren und das sie auch direkter zu verstehen sind als Zugriff auf Stelle 0 und 1, da man wirklich gar nicht darüber nachdenken muss. Und die Zeile Code spart man sich dann auch nicht, wenn man stattdessen einen Kommenatr schreibt, der dann nur an der einen Stelle und nicht bei jeder Verwendung steht.

sarahvgls

Also ich finde es super übersichtlich und richtig toll so, vielen Dank!! :D

sarahvgls · 2024-11-24T20:35:57Z

protzilla/constants/colors.py

+
+PLOT_SECONDARY_COLOR = PLOT_COLOR_SEQUENCE[1]
+"""Second color in list."""


Finde ich richtig schön so!!!

noLeonardo

Sieht insgesamt alles viel ordentlicher, einheitlicher und verständlicher aus und funktioniert bei mir auch :)

ronjakrg added 2 commits November 15, 2024 13:06

added path constants to export workflow message

b78fc15

removed unnecessary import

eda0ccf

ronjakrg added 7 commits November 17, 2024 21:24

added plotly template, included template in data preprocessing plots

ed8fc8f

fixed attribute error

ebb73cb

set plotly_protzilla as default template

5c0ddf8

removed manual template assignments

81df44b

removed plotly tools and changed dragmode

ce1861e

fixed typo in clustergram form

ce7489f

renamed color constants

56262f3

ronjakrg marked this pull request as ready for review November 19, 2024 22:49

ronjakrg requested review from noLeonardo and gitjannes November 21, 2024 09:50

gitjannes approved these changes Nov 21, 2024

View reviewed changes

sarahvgls requested changes Nov 21, 2024

View reviewed changes

added requested changes

a4c087c

ronjakrg requested a review from sarahvgls November 23, 2024 17:45

ronjakrg self-assigned this Nov 24, 2024

sarahvgls approved these changes Nov 24, 2024

View reviewed changes

noLeonardo approved these changes Nov 25, 2024

View reviewed changes

ronjakrg merged commit d9941ce into dev Nov 25, 2024
1 check passed

ronjakrg deleted the plotly-configurations branch November 25, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Plotly Configurations #544

Adding Plotly Configurations #544

ronjakrg commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

gitjannes left a comment

sarahvgls left a comment

sarahvgls Nov 21, 2024

ronjakrg Nov 21, 2024

sarahvgls Nov 21, 2024

sarahvgls Nov 21, 2024

sarahvgls Nov 21, 2024

sarahvgls Nov 21, 2024

sarahvgls Nov 21, 2024

sarahvgls Nov 21, 2024

ronjakrg commented Nov 21, 2024

sarahvgls commented Nov 22, 2024

sarahvgls left a comment

sarahvgls Nov 24, 2024

noLeonardo left a comment


		PLOT_SECONDARY_COLOR = PLOT_COLOR_SEQUENCE[1]
		"""Second color in list."""

Adding Plotly Configurations #544

Adding Plotly Configurations #544

Conversation

ronjakrg commented Nov 15, 2024 • edited Loading

Description

Changes

Testing

PR checklist

github-actions bot commented Nov 15, 2024 • edited Loading

Coverage report

gitjannes left a comment

Choose a reason for hiding this comment

sarahvgls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronjakrg commented Nov 21, 2024

Kompromiss

sarahvgls commented Nov 22, 2024

sarahvgls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noLeonardo left a comment

Choose a reason for hiding this comment

ronjakrg commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading