perses icon indicating copy to clipboard operation
perses copied to clipboard

Duplicate instances of `self._setup_complex_phase()` in relative_setup.py

Open zhang-ivy opened this issue 1 year ago • 2 comments

I noticed that self._setup_complex_phase() is called twice.

  • first time: https://github.com/choderalab/perses/blob/main/perses/app/relative_setup.py#L333 -- as noted in the comments, this call needs to happen before the system generator is created because for host guest systems, the host molecule needs to be supplied to when creating the system generator
  • second time: https://github.com/choderalab/perses/blob/main/perses/app/relative_setup.py#L409 -- not sure if this is necessary, try removing it and seeing what happens?

zhang-ivy avatar Jul 28 '23 16:07 zhang-ivy

Iván notes that this is not super high priority since we are in the midst of overhauling the API anyway

zhang-ivy avatar Jul 28 '23 16:07 zhang-ivy

Yes,the comment on https://github.com/choderalab/perses/blob/ae5580173e321cc85014a2a780080b9b546f18bf/perses/app/relative_setup.py#L329 explains why we needed that at that instance but, as discussed, we can quickly investigate what happens if we remove the second call, since that seems redundant and it shouldn't be doing anything new.

As a reference, when I worked in the refactor for the NEQ cycling protocol, the RelativeFEPSetup.__init__ method of is more organized and it's only setting up the phases at the end of it, as per https://github.com/choderalab/perses/blob/faac03085376d8aace61d41e8b53d1e8e6e2f035/perses/app/relative_setup.py#L349 (just to keep in mind).

ijpulidos avatar Jul 28 '23 17:07 ijpulidos