[WIP] Fe setup structure remodelling #346

RiesBen · 2024-09-11T09:50:16Z

This is a draft!

The idea of the draft is to make gufe future ready for future developments and to indictate the power of certain approaches as they are more general than currently indicated.

Example:
so network planners are not really dependent on LigandAtomMappings, but ComponentMappings, that could be any type of component.

_Note: _

This PR was discussed in the protocol dev round.
Changes are only in setup, rest is just refactoring.

Principles:

Changes should not be API-breaking.

Thoughts:

A ChemicalSystem could be interpreted as a collection of Components and therefore this should be reflected in the definitions.

pep8speaks · 2024-09-11T09:50:29Z

Hello @RiesBen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file gufe/__init__.py:

Line 34:80: E501 line too long (86 > 79 characters)

In the file gufe/setup/alchemical_network_planner.py:

Line 11:80: E501 line too long (82 > 79 characters)

In the file gufe/setup/chemical_system_generator.py:

Line 11:1: E302 expected 2 blank lines, found 1
Line 20:80: E501 line too long (81 > 79 characters)
Line 25:36: W292 no newline at end of file

In the file gufe/setup/network_planning/atom_mapping_based/atom_mapper.py:

Line 31:1: W391 blank line at end of file

In the file gufe/setup/network_planning/atom_mapping_based/atom_mapping.py:

Line 59:1: W391 blank line at end of file

In the file gufe/setup/network_planning/atom_mapping_based/atom_mapping_scorer.py:

Line 28:80: E501 line too long (136 > 79 characters)
Line 40:80: E501 line too long (89 > 79 characters)

In the file gufe/setup/network_planning/atom_mapping_based/ligandnetwork.py:

Line 15:80: E501 line too long (96 > 79 characters)
Line 281:12: W292 no newline at end of file
Line 281:12: W292 no newline at end of file
Line 281:12: W292 no newline at end of file

In the file gufe/setup/network_planning/component_mapping.py:

Line 20:80: E501 line too long (109 > 79 characters)

In the file gufe/setup/network_planning/component_mapping_scorer.py:

Line 7:5: E271 multiple spaces after keyword
Line 9:1: E302 expected 2 blank lines, found 1
Line 24:80: E501 line too long (111 > 79 characters)
Line 34:80: E501 line too long (89 > 79 characters)

In the file gufe/setup/network_planning/network_plan.py:

Line 16:1: E302 expected 2 blank lines, found 1
Line 127:31: E127 continuation line over-indented for visual indent
Line 188:80: E501 line too long (80 > 79 characters)

In the file gufe/setup/network_planning/network_planner.py:

Line 12:1: E302 expected 2 blank lines, found 1
Line 19:55: E231 missing whitespace after ':'
Line 19:80: E501 line too long (95 > 79 characters)
Line 20:80: E501 line too long (97 > 79 characters)
Line 29:22: E222 multiple spaces after operator
Line 32:5: E303 too many blank lines (2)
Line 32:40: E225 missing whitespace around operator
Line 36:80: E501 line too long (94 > 79 characters)
Line 36:81: E225 missing whitespace around operator

Comment last updated at 2024-09-18 08:20:34 UTC

there are few discussion points left.

dotsdl · 2024-09-25T06:01:19Z

@RiesBen please let me know when you'd like a review from me! Happy to help you get this into gufe!

IAlibay · 2024-09-25T12:14:58Z

gufe/__init__.py

@@ -16,12 +16,6 @@

 from .chemicalsystem import ChemicalSystem

-from .mapping import (


We can't move all these imports, this will break the API

IAlibay · 2024-09-25T12:15:09Z

gufe/mapping/__init__.py

-# This code is part of gufe and is licensed under the MIT license.
-# For details, see https://github.com/OpenFreeEnergy/gufe
-"""Defining the relationship between different components"""
-from .componentmapping import ComponentMapping


Similarly, this can't go away until 2.0

IAlibay · 2024-09-25T12:16:58Z

gufe/setup/chemical_system_generator.py

+
+# Todo: connect to protocols - use this for labels?
+
+class RFEComponentLabels(str, Enum):


This needs more discusison, I don't remember this being in the plans for updating things, definitely using labels for components seems like something we don't want to do.

IAlibay · 2024-09-25T12:18:04Z

gufe/setup/network_planning/atom_mapping_based/atom_mapper.py

@@ -27,4 +27,5 @@ def suggest_mappings(self,
 Suggests zero or more :class:`.AtomMapping` objects, which are possible
 atom mappings between two :class:`.Component` objects.
 """
- ...
+ raise NotImplementedError("This function was not implemented.")


I believe this (...) is a pattern we use quite widely for our abcs?

IAlibay · 2024-09-25T12:18:14Z

gufe/setup/network_planning/atom_mapping_based/atom_mapping.py

@@ -37,22 +37,23 @@ def componentA_to_componentB(self) -> Mapping[int, int]:
 entity in the other component (e.g. the atom disappears), therefore
 resulting in a KeyError on query
 """
- ...
+ raise NotImplementedError("This function was not implemented.")


IAlibay · 2024-09-25T12:18:40Z

gufe/setup/network_planning/atom_mapping_based/atom_mapping_scorer.py

+from .atom_mapping import AtomMapping
+
+
+class AtomMappingScorer(GufeTokenizable):


This is in line with what we discussed, so this feature I think it ok to include.

IAlibay · 2024-09-25T12:19:13Z

gufe/setup/network_planning/component_mapper.py

+from .component_mapping import ComponentMapping
+
+
+class ComponentMapper(GufeTokenizable):


This is also in line with what was discussed.

IAlibay · 2024-09-25T12:20:30Z

gufe/setup/network_planning/network_plan.py

+
+from .component_mapping import ComponentMapping
+
+class NetworkPlan(GufeTokenizable):


Very briefly looking at class names, I don't think Plan is the right word here. It's not immediately clear what it does.

dotsdl · 2024-10-04T14:24:45Z

@RiesBen can you offer a response to @IAlibay above? Can you give us a checklist of things you aim to do before you declare this PR ready for review?

RiesBen added 2 commits August 31, 2024 22:02

initial drafting

8c59863

initial drafting

c13f414

RiesBen linked an issue Sep 11, 2024 that may be closed by this pull request

Build ComponentMapping, ComponentMappingScorer #342

Open

This was referenced Sep 11, 2024

Adding a generic atom mapping scorer #306

Closed

Adding NetworkPlanner #307

Closed

Merge branch 'main' into FE_setup_Structure

c5839b7

RiesBen changed the title ~~[WIP] Fe setup structure~~ [WIP] Fe setup structure remodelling Sep 11, 2024

RiesBen self-assigned this Sep 11, 2024

RiesBen added the enhancement New feature or request label Sep 11, 2024

dotsdl self-requested a review September 11, 2024 21:11

minor improvements to the draft.

2a243c8

there are few discussion points left.

dotsdl added this to the Release 1.1 milestone Sep 25, 2024

IAlibay requested changes Sep 25, 2024

View reviewed changes

dotsdl modified the milestones: Release 1.1, Release 2.0.0 Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fe setup structure remodelling #346

[WIP] Fe setup structure remodelling #346

RiesBen commented Sep 11, 2024 •

edited

Loading

pep8speaks commented Sep 11, 2024 •

edited

Loading

dotsdl commented Sep 25, 2024

IAlibay Sep 25, 2024

IAlibay Sep 25, 2024

IAlibay Sep 25, 2024

IAlibay Sep 25, 2024

IAlibay Sep 25, 2024

IAlibay Sep 25, 2024

IAlibay Sep 25, 2024

IAlibay Sep 25, 2024

dotsdl commented Oct 4, 2024

		@@ -16,12 +16,6 @@

		from .chemicalsystem import ChemicalSystem

		from .mapping import (


		# Todo: connect to protocols - use this for labels?

		class RFEComponentLabels(str, Enum):

		from .atom_mapping import AtomMapping


		class AtomMappingScorer(GufeTokenizable):

		from .component_mapping import ComponentMapping


		class ComponentMapper(GufeTokenizable):


		from .component_mapping import ComponentMapping

		class NetworkPlan(GufeTokenizable):

[WIP] Fe setup structure remodelling #346

Are you sure you want to change the base?

[WIP] Fe setup structure remodelling #346

Conversation

RiesBen commented Sep 11, 2024 • edited Loading

pep8speaks commented Sep 11, 2024 • edited Loading

Comment last updated at 2024-09-18 08:20:34 UTC

dotsdl commented Sep 25, 2024

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

dotsdl commented Oct 4, 2024

RiesBen commented Sep 11, 2024 •

edited

Loading

pep8speaks commented Sep 11, 2024 •

edited

Loading