Refactoring details #570

Jonas0000 · 2025-01-26T22:42:46Z

Description

refactoring of details page ended up in refactoring steps.

Plot and calculate forms are combined into one so that calculating and plotting is possible by one click.
Handling of methods is overworked.
The definition of input keys is done by the method parameters to have less duplicated code.

There is still some stuff left to improve but i want to concentrate on the migration to react.

Tested a lot of steps including the MVP steps.

github-actions · 2025-02-11T21:09:22Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
protzilla
disk_operator.py
run.py
run_helper.py
steps.py					42, 76-77, 81-82, 89, 93, 97, 137, 139, 144, 159, 161, 169-170, 172-175, 177, 179, 181, 192, 204, 215, 220, 230, 236, 260
protzilla/data_analysis
plots.py
protein_graphs.py
ptm_analysis.py					56, 123-125
protzilla/data_integration
database_query.py					119
di_plots.py
enrichment_analysis.py
protzilla/data_preprocessing
imputation.py					91
plots.py
protzilla/importing
ms_data_import.py					122, 276
protzilla/methods
data_analysis.py					21, 41, 65, 88, 111, 123, 137, 166, 177, 189, 200, 212, 238, 246-257, 287, 303, 341, 348, 367, 408, 430, 452, 487, 502, 524, 542, 562, 581, 597, 615, 624, 636, 645, 651
data_integration.py					22
protzilla/utilities
utilities.py					184
ui/main
settings.py					131
ui/runs
fields.py
views.py					21-24, 73-74, 180, 257, 678, 683-687
views_helper.py
ui/runs/forms
base.py					107
custom_fields.py					119
data_analysis.py					323-327, 384, 414, 450, 473, 1188, 1226, 1248
data_integration.py
ui/settings
plot_template.py					84
Project Total

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

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

maximilianKalff

Finde es cool, wie jetzt calculate und plot zusammen gehandelt werden. Ich hatte noch ein paar Fragen an manchen Stellen, aber die habe ich alle dazugeschrieben. :))

maximilianKalff · 2025-02-17T13:17:26Z

protzilla/data_analysis/classification.py

Warum werden die Parameter alle so spezifiziert? Damit grenzt man ja die Flexibilität der Funktionen schon ein, weil man jetzt nicht mehr alle Parameter, die der Random Forest Classifier von sklearn bietet, nutzen kann.

Ich habe eigentlich versucht, dass keine Funktionalität oder Flexibiltät verloren geht, sondern nur die **kwargs ausspezifiziert, wenn es konkrete Parameter sind, die verwendet werden oder entfernt, wenn sie überhaupt nicht genutzt werden. Wenn ich etwas übersehen habe, dann ändere ich das gerne. Kannst du mir dafür eine konkrete stelle nennen, an der Flexibilität verloren geht?
Zum Hintergrund, warum ich das gemacht habe: Ich fand es sehr unverständlich, was für parameter bei den Kwargs verwendet erwartet werden und missverständlich, wenn die Funktion kwargs angenommen hat, obwohl mit diesen nichts getan wird. So wollte ich in den Funktionen klarer machen, welches eigentlich die Parameter sind, die genutzt werden.

maximilianKalff · 2025-02-17T13:23:29Z

protzilla/data_preprocessing/filter_proteins.py

Ich vermute die build_pie_bar_plot Funktion wurde sonst nirgends aufgerufen? Aber ist es vielleicht nicht sinnvoll, wenn man eine generische allgemeine Funktion für diese Art von Plot hat? Für den Fall, dass man sie nochmal verwenden möchte. Also warum hast du das zusammengeführt?

Grundsätzlich bin ich voll bei dir und fände eine allgemeine Funktion auch sehr sinnvoll. Allerdings ist die Funktion an dieser stelle so spezifisch (Bspw. durch sie Labels ["Proteins kept", "Proteins filtered"]), dass man sie sowieso kein zweites mal verwenden kann. Hintergrund war einfach nur, dass ich die zweite Funktion, die nichts tut, entfernen wollte, weil sie nichts tut.

Eigentlich ist die Funktion auch genau so gut an anderer Stelle wieder verwendbar, wie vorher, dadurch, dass ich nur Parameter umbenannt habe.

Fändest du es trotzdem besser, wenn ich wieder eine zusätzliche Funktion einfüge?

protzilla/methods/data_analysis.py

protzilla/methods/data_integration.py

ui/runs/form_mapping.py

maximilianKalff · 2025-02-17T13:41:44Z

ui/runs/forms/data_integration.py

Sollen die Sachen auskommentiert bleiben?

Gerade festgestellt, dass die Felder zwar nicht normal verwendet werden, dafür aber an anderer Stelle genutzt werden und daher habe ich sie wieder ein kommentiert.

ui/runs/forms/data_preprocessing.py

Jonas0000

Hab auf die ersten Kommentare geantwortet. Rest kommt gleich :D

Jonas0000 · 2025-02-24T13:34:02Z

protzilla/data_analysis/classification.py

Ich habe eigentlich versucht, dass keine Funktionalität oder Flexibiltät verloren geht, sondern nur die **kwargs ausspezifiziert, wenn es konkrete Parameter sind, die verwendet werden oder entfernt, wenn sie überhaupt nicht genutzt werden. Wenn ich etwas übersehen habe, dann ändere ich das gerne. Kannst du mir dafür eine konkrete stelle nennen, an der Flexibilität verloren geht?
Zum Hintergrund, warum ich das gemacht habe: Ich fand es sehr unverständlich, was für parameter bei den Kwargs verwendet erwartet werden und missverständlich, wenn die Funktion kwargs angenommen hat, obwohl mit diesen nichts getan wird. So wollte ich in den Funktionen klarer machen, welches eigentlich die Parameter sind, die genutzt werden.

Jonas0000 · 2025-02-24T14:00:03Z

protzilla/data_preprocessing/filter_proteins.py

Grundsätzlich bin ich voll bei dir und fände eine allgemeine Funktion auch sehr sinnvoll. Allerdings ist die Funktion an dieser stelle so spezifisch (Bspw. durch sie Labels ["Proteins kept", "Proteins filtered"]), dass man sie sowieso kein zweites mal verwenden kann. Hintergrund war einfach nur, dass ich die zweite Funktion, die nichts tut, entfernen wollte, weil sie nichts tut.

Eigentlich ist die Funktion auch genau so gut an anderer Stelle wieder verwendbar, wie vorher, dadurch, dass ich nur Parameter umbenannt habe.

Fändest du es trotzdem besser, wenn ich wieder eine zusätzliche Funktion einfüge?

protzilla/methods/data_analysis.py

protzilla/methods/data_integration.py

Jonas0000

Fertig, @maximilianKalff schaue gerne nochmal rüber, ob das jetzt so für dich passt bzw. du bei den ersten Kommentaren auch mit der Erklärung als Backgroundinfo eine Änderung haben möchtest.
@sarahvgls ich denke, dass du jetzt schon drüber schauen kannst, wenn du möchtest.

Jonas0000 · 2025-02-24T14:32:39Z

ui/runs/forms/data_integration.py

Gerade festgestellt, dass die Felder zwar nicht normal verwendet werden, dafür aber an anderer Stelle genutzt werden und daher habe ich sie wieder ein kommentiert.

sarahvgls

Ein paar Anmerkungen von meiner Seite:

Super cool, dass du das alles gemacht hast. Also wirklich, meinen absoluten Respekt!!!
Ich finde einige der renames wirklich sinnvoll und du vernichtest echt einige Code smells, sehr sehr gut!
zu den kwargs:
Ich habe mich dazu eben auch etwas eingelesen, was denn eigentlich so üblich ist, vor allem, da ich **kwargs schon recht häufig treffe und daher anfangs auch eher die Meinung vertreten wollte, dass wir die doch lieber behalten wollen. Aber muss dir dann doch eigentlich zustimmen, dass ein sinnvolles weitergeben dieser Parameter einfach ein code smell ist und nicht zur verständlichkeit beiträgt. @maximilianKalff Sag gerne noch mal Bescheid, wenn dir was einfällt, warum es doch so besser wäre (vielleicht übersehen Jonas und ich ja gerade was). Ich könnte mich aber auch gut damit anfreunden, dass man so die vielen Parameter doch direkt sieht und - sollte es Bedarf geben dies zu erweitern, man einfach mehr Parameter hinzufügt und diese dann halt verwendet. Aber ein intransparentes **kwargs klingt hier schon eher nach einer Unsicherheit, was man eigentlich übergibt.
Das anders-behandeln von Input parameters ist so viel smarter, das ist echt eine Verbesserung!!
Ich habe ein paar Todos gefunden, sag gerne mal bescheid ob du die noch schnell angreifst, ansonsten gerne ein Issue dafür aufmachen
Ich hab ein paar Fragen und Anmerkungen im Code dazu geschrieben, sag mir gerne mal, was du so davon hältst
den html teil hab ich tbh geskippt

Und damit beende ich mal meinen review-Spam hier, vielen Dank für deine Arbeit!!!

protzilla/data_analysis/classification.py

protzilla/data_analysis/classification_helper.py

protzilla/data_preprocessing/filter_samples.py

protzilla/data_preprocessing/imputation.py

protzilla/data_preprocessing/transformation.py

protzilla/steps.py

tests/protzilla/data_preprocessing/test_filter_proteins.py

Jonas0000 · 2025-02-26T10:57:52Z

@sarahvgls ich sollte jetzt alle deine Kommentare bearbeitet haben. Vielen Dank für die vielen Hinweise und sinnvollen Kommentare. Könntest du nochmal rüber schauen und Kommentare als erledigt markieren, wenn sie aus deiner Sicht erledigt sind. Falls ich bei der Menge etwas übersehen habe, sag gerne nochmal Bescheid

sarahvgls

Noch ein paar kleine Anmerkungen, sonst sollte alles passen :))

protzilla/data_analysis/classification.py

protzilla/data_analysis/classification_helper.py

protzilla/data_preprocessing/transformation.py

sarahvgls · 2025-02-26T17:02:58Z

protzilla/methods/data_analysis.py



-class PlotVolcano(PlotStep):
+class PlotVolcano(DataAnalysisStep):


Blöde frage vielleicht aber ist es dann nicht irgendwie unvollständig, dass wir noch ein paar PlotSteps haben obwohl die eigentlich entfernt werden sollen?

protzilla/methods/data_analysis.py

protzilla/methods/data_integration.py

protzilla/steps.py

tests/protzilla/data_preprocessing/test_filter_proteins.py

sarahvgls · 2025-02-26T17:25:41Z

Und mir fällt gerade auf, dass der eine Test nach deinen letzten Änderungen wohl leider failt weil wohl kwargs fehlen. Wäre super, wenn du das noch wieder in den Tests anpassen könntest :))

Jonas0000 · 2025-03-01T11:05:37Z

@maximilianKalff Helloo, könntest du nochmal über deine Kommentare schauen?

Jonas0000 added 10 commits December 11, 2024 16:57

removed unused code

5d3ce27

Merge branch 'dev' into refactoring_details

a520ef5

refactor views

b8fb75d

combined forms

2b9d03b

forms combined, preprocessing migrated

f336351

more refactoring

97e8425

fix list, remove print, corrected MetadataImportMethodDiann

dab49a3

corrected GO method

7f0533e

Merge branch 'dev' into refactoring_details

4249176

corrected test

6dcb58d

Jonas0000 marked this pull request as draft January 27, 2025 22:45

test fixes

b720c67

Jonas0000 marked this pull request as ready for review February 11, 2025 21:12

maximilianKalff reviewed Feb 17, 2025

View reviewed changes

TODO removed

d7e0f58

Jonas0000 commented Feb 24, 2025

View reviewed changes

removed unused _forward_mapping_plots and usages

cfeb535

Jonas0000 commented Feb 24, 2025

View reviewed changes

uncommend needed code

f18060a

sarahvgls requested changes Feb 24, 2025

View reviewed changes

Jonas0000 added 2 commits February 25, 2025 16:47

Merge branch 'dev' into refactoring_details

f0c53c5

cleaning up the code

a5bd23c

sarahvgls reviewed Feb 26, 2025

View reviewed changes

Jonas0000 force-pushed the refactoring_details branch from 26c87be to a5bd23c Compare March 1, 2025 11:21

Jonas0000 added 2 commits March 1, 2025 12:32

doc string updated

edf8c6d

fix kwarg parameter

e3e6427

Jonas0000 added 3 commits March 3, 2025 22:56

fix test

2d5dc83

fix

7c22be0

skipping enrichr tests where apis dosnt work

2c0d874

Jonas0000 merged commit e306590 into dev Mar 4, 2025
1 check passed



		class PlotVolcano(PlotStep):
		class PlotVolcano(DataAnalysisStep):

Refactoring details #570

Refactoring details #570

Uh oh!

Conversation

Jonas0000 commented Jan 26, 2025

Description

Uh oh!

github-actions bot commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

maximilianKalff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jonas0000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Jonas0000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahvgls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jonas0000 commented Feb 26, 2025

Uh oh!

sarahvgls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarahvgls commented Feb 26, 2025

Uh oh!

Jonas0000 commented Mar 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

github-actions bot commented Feb 11, 2025 •

edited

Loading