Drop class name analogies favoring technical names #21

rvhonorato · 2021-07-13T10:22:36Z

This is a boring PR to make the code more accessible.

I do believe analogy is a great technique to match unrelated objects in order facilitate understanding and it is seen in many large projects out there in the wild. However me and @joaomcteixeira are getting lost in the ones that were present here since they were often actually quite metaphoric.

Additionally, since we aim the project to have a wide reach, we need to also consider code accessibility and making it more boring can increase its readability. There will still be plenty of room for creative expression later on :)

With this PR I also linted to code to comply with E501.
In this branch there are no tests yet, so I tested it by running the examples and checking if things were working.

brianjimenez · 2021-07-13T10:29:51Z

haddock/defaults.py

-        "trans_vector_48": data_path / "toppar/initial_positions/trans_vector_48",
-        "trans_vector_49": data_path / "toppar/initial_positions/trans_vector_49",
-        "trans_vector_50": data_path / "toppar/initial_positions/trans_vector_50"
+        "trans_vector_0": (data_path /


This is a classic example where I would not personally follow strict 80-lenght line except if committing a major number of lines is intended in order to inflate statistics.

agreed. it actually reads better with single lines despite going over 80 chars which I like so much. Nonetheless what about:

use Path and not /.

Don't overload operators 😉

TRANSLATION_VECTORS = { f"trans_vector_{i}": Path(data_path, 'toppar', 'initial_positions', f'trans_vector_{i}') for i in range(51) }

ooh this is great @joaomcteixeira, thanks!

brianjimenez · 2021-07-13T10:30:53Z

haddock/modules/sampling/lightdock.py

@@ -27,34 +27,43 @@ def run(self, module_information):
        self.patch_defaults(module_information)

        # Get the models generated in previous step
-        models_to_score = [p for p in self.previous_io.output if p.file_type == Format.PDB]


List comprehension is faster and IMHO more readable here.

I agree. please keep the list comprehension.

brianjimenez · 2021-07-13T10:31:56Z

haddock/modules/sampling/lightdock.py


        # Check if multiple models are provided
        if len(models_to_score) > 1:
-            self.finish_with_error("Only one model allowed in LightDock sampling module")
+            self.finish_with_error("Only one model allowed in LightDock"


Same non-sense 80-length limitation. Luckily, not using old screens anymore.

in these cases if you really don't wanna go over 80 I advise writing:

_msg = "blablabla" self.finish_with_error(_msg)

or

self.finish_with_error( "blablablabla 1" "blablabbla 2" )

brianjimenez · 2021-07-13T10:33:12Z

haddock/modules/topology/default.py

            expected = []
            not_found = []
            for model in models:
                model_name = model.stem
-                if not (processed_pdb := self.path / f"{model_name}_haddock.{Format.PDB}").is_file():


Why this? Using modern Python and walrus operator is not a crime...

brianjimenez · 2021-07-13T10:34:22Z

haddock/ontology.py

@@ -29,7 +30,9 @@ def __init__(self, file_name, file_type, path='.'):
        self.path = str(Path(path).parent.absolute())

    def __repr__(self):
-        return f"[{self.file_type}|{self.created}] {Path(self.path) / self.file_name}"
+        rep = (f"[{self.file_type}|{self.created}] "


Less efficient and unnecessary.

brianjimenez · 2021-07-13T10:35:22Z

haddock/workflow.py

+            step.execute()
+
+
+class Recipe:


If not using metaphors anymore, maybe there is a better name for this class too?

brianjimenez

Boring review too. I am not a fan of strict linting, specially of line-length rules, but your call. List comprehensions and walrus operator are more pythonic options though.

joaomcteixeira

Just made general comments. Nothing critical. Somethings I personally dislike, but they are not mistakes either.

joaomcteixeira · 2021-07-13T10:39:24Z

haddock/defaults.py

-        "trans_vector_48": data_path / "toppar/initial_positions/trans_vector_48",
-        "trans_vector_49": data_path / "toppar/initial_positions/trans_vector_49",
-        "trans_vector_50": data_path / "toppar/initial_positions/trans_vector_50"
+        "trans_vector_0": (data_path /


agreed. it actually reads better with single lines despite going over 80 chars which I like so much. Nonetheless what about:

use Path and not /.

Don't overload operators 😉

TRANSLATION_VECTORS = { f"trans_vector_{i}": Path(data_path, 'toppar', 'initial_positions', f'trans_vector_{i}') for i in range(51) }

joaomcteixeira · 2021-07-13T10:40:38Z

haddock/modules/sampling/lightdock.py

@@ -27,34 +27,43 @@ def run(self, module_information):
        self.patch_defaults(module_information)

        # Get the models generated in previous step
-        models_to_score = [p for p in self.previous_io.output if p.file_type == Format.PDB]


I agree. please keep the list comprehension.

joaomcteixeira · 2021-07-13T10:42:25Z

haddock/modules/sampling/lightdock.py


        # Check if multiple models are provided
        if len(models_to_score) > 1:
-            self.finish_with_error("Only one model allowed in LightDock sampling module")
+            self.finish_with_error("Only one model allowed in LightDock"


in these cases if you really don't wanna go over 80 I advise writing:

_msg = "blablabla" self.finish_with_error(_msg)

or

self.finish_with_error( "blablablabla 1" "blablabbla 2" )

joaomcteixeira · 2021-07-13T10:43:40Z

haddock/modules/sampling/lightdock.py


        model = models_to_score[0]
        # Check if chain IDs are present
-        segids, chains = PDBFactory.identify_chainseg(Path(model.path) / model.file_name)
+        segids, chains = PDBFactory.identify_chainseg(Path(model.path) /


humble suggestion since you are refactoring enforcing 80 chars

_path = Path(model.path, model.file_name) PDBFactory.indentify_chainseg(_path)

same everywhere. in my opinion. mixing operator overload with line breaks is difficult to read

joaomcteixeira · 2021-07-13T10:45:24Z

haddock/modules/scoring/default.py

@@ -51,11 +56,21 @@ def run(self, module_information):
        jobs = []

        # Get the models generated in previous step
-        models_to_score = [p for p in self.previous_io.output if p.file_type == Format.PDB]
+        models_to_score = []


same as before.
if you want to be purist, these logic should be some kind of method of function. but for now just keep the list comprehension

joaomcteixeira · 2021-07-13T11:09:55Z

haddock/workflow.py

+                raise HaddockError from re
+
+
+class Step:


Needs documentation

joaomcteixeira · 2021-07-13T11:12:50Z

However, despite we change the class names to more technical ones we should keep the nice HADDOCK-related interface for the users. Phrases of sailors are nice ⛵

Some names `flavour` remained after haddocking#21. This broke any runs where flavour was not the "default", for example in lightdock.

Update installation instructions after #21

rvhonorato added 15 commits July 13, 2021 12:07

Update haddock3.py

61a2d5d

Update scoring.toml

1b50b54

Update engine.py

bec6b3b

Update topology.py

e971823

Update defaults.py

1176bed

Update lightdock.py

de69c8c

Update default.py

41bcfd5

Update default.py

f97b5e5

Update ontology.py

b563af8

Update parallel.py

fb31138

Update pdbutil.py

39883c3

Update structure.py

7fb810e

Update version.py

ef3c1c5

Create workflow.py

d0dffa3

Update .gitignore

79b27cf

rvhonorato requested review from joaomcteixeira and brianjimenez July 13, 2021 10:22

rvhonorato self-assigned this Jul 13, 2021

brianjimenez reviewed Jul 13, 2021

View reviewed changes

joaomcteixeira reviewed Jul 13, 2021

View reviewed changes

joaomcteixeira changed the title ~~Boring refactor~~ Drop class name analogies favoring technical names Jul 13, 2021

joaomcteixeira reviewed Jul 13, 2021

View reviewed changes

haddock/workflow.py

raise HaddockError from re

class Step:

Copy link

Member

joaomcteixeira Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs documentation

address reviews

bd45a10

rvhonorato merged commit 8b8b8eb into haddocking:beta1 Jul 13, 2021

rvhonorato deleted the boring branch July 13, 2021 18:26

This was referenced Jul 16, 2021

data_path not being found #24

Merged

Update installation instructions #25

Merged

joaomcteixeira added a commit to joaomcteixeira/haddock3 that referenced this pull request Jul 16, 2021

renames flavour to mode

161b7cc

Some names `flavour` remained after haddocking#21. This broke any runs where flavour was not the "default", for example in lightdock.

joaomcteixeira mentioned this pull request Jul 16, 2021

renames flavour to mode missing files #26

Merged

joaomcteixeira added a commit that referenced this pull request Jul 19, 2021

Merge pull request #25

667689b

Update installation instructions after #21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop class name analogies favoring technical names #21

Drop class name analogies favoring technical names #21

rvhonorato commented Jul 13, 2021

brianjimenez Jul 13, 2021

joaomcteixeira Jul 13, 2021

rvhonorato Jul 13, 2021

brianjimenez Jul 13, 2021

joaomcteixeira Jul 13, 2021

brianjimenez Jul 13, 2021

joaomcteixeira Jul 13, 2021

brianjimenez Jul 13, 2021

brianjimenez Jul 13, 2021

brianjimenez Jul 13, 2021

brianjimenez left a comment

joaomcteixeira left a comment

joaomcteixeira Jul 13, 2021

joaomcteixeira Jul 13, 2021

joaomcteixeira Jul 13, 2021

joaomcteixeira Jul 13, 2021

joaomcteixeira Jul 13, 2021

joaomcteixeira Jul 13, 2021

joaomcteixeira commented Jul 13, 2021

Drop class name analogies favoring technical names #21

Drop class name analogies favoring technical names #21

Conversation

rvhonorato commented Jul 13, 2021

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

Choose a reason for hiding this comment

brianjimenez left a comment

Choose a reason for hiding this comment

joaomcteixeira 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

joaomcteixeira commented Jul 13, 2021