Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gufe: Network Planning Stage - make this part ready for new methods. #346

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

RiesBen
Copy link
Contributor

@RiesBen RiesBen commented Sep 11, 2024

This PR is mainly motifvated by #342 (pls. read this issue too ;) ).

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 by the Types we use.

Example:

so network planners are not really dependent on LigandAtomMappings, but ComponentMappings, that could be any type of component. Currently we are limiting the signatures to the RBFE Small Molecules. With this PR, we make it possible to show the generality of approaches

My train of thought follows in general the Setup Pipeline that we defined back in the day, but a bit more abstract than in the following Ligand-RBFE workflow. With these changes we get setup ready for e.g. ProteinMutations and SepTop.

RBFE_setup_pipeline drawio

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.

Additional Thoughts:

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

@pep8speaks
Copy link

pep8speaks commented Sep 11, 2024

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

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

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

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

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

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

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

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

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

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)

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)

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

@RiesBen RiesBen changed the title [WIP] Fe setup structure [WIP] Fe setup structure remodelling Sep 11, 2024
@RiesBen RiesBen self-assigned this Sep 11, 2024
@RiesBen RiesBen added the enhancement New feature or request label Sep 11, 2024
@dotsdl dotsdl self-requested a review September 11, 2024 21:11
there are few discussion points left.
@dotsdl
Copy link
Member

dotsdl commented Sep 25, 2024

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

@dotsdl dotsdl added this to the Release 1.1 milestone Sep 25, 2024
gufe/__init__.py Show resolved Hide resolved
# 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
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this can't go away until 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm would it make sense to split this out into a new PR, or drop it completly? I m not sure of your intend here.

Copy link
Member

Choose a reason for hiding this comment

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

The intent of this comment is solely "API breaks can't happen until 2.0 and this is an API break".

As for the other things, I would suggest keeping a PR that sticks to changes that we had agreed upon and then doing a separate PR for any other extras.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think now it should be not api breaking.

gufe/setup/chemical_system_generator.py Outdated Show resolved Hide resolved
gufe/setup/network_planning/network_plan.py Show resolved Hide resolved
@dotsdl dotsdl modified the milestones: Release 1.1, Release 2.0.0 Oct 3, 2024
@dotsdl
Copy link
Member

dotsdl commented Oct 4, 2024

@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
Copy link
Contributor Author

RiesBen commented Oct 15, 2024

@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?

Sure :)
I think the current ToDo List is the following:

  • discuss change suggestions in order to find a clear direction with some changes
    • RFEComponentLabels Labels for ChemicalSystems transfer them from OpenFE to here or not?
    • NetworkPlan name improvement
    • How to proceed with code/import structure. my suggestion, make them not API breaking
  • adapt the PR to the discussions.
  • Check and adapt unit test
    • AtomMappingScorer
    • ComponentMappingScorer
    • ComponentMapper
    • NetworkPlan
    • Network_planner
      --> after this ready to merge :)

@RiesBen RiesBen linked an issue Oct 16, 2024 that may be closed by this pull request
@RiesBen RiesBen changed the title [WIP] Fe setup structure remodelling gufe: Network Planning Stage - make this part ready for new methods. Oct 17, 2024
@RiesBen
Copy link
Contributor Author

RiesBen commented Oct 22, 2024

Current CI-Failures reasons are 2 tests:

  1. Gufe key stability: the key for LigandNetwork and LigandAtomMapping is not equal to the expected GufeKey. (affects: test_key_stable, test_repr
  2. test_graph_annotations: The Annotations of the LigandAtomMapping is lost, after testing found this to have some weird behavior when you call ligand_network.edges and `ligand_network.graph..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
5 participants