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

New Class: Ring Closing Vertex #53

Open
einarkjellback opened this issue Apr 22, 2021 · 1 comment
Open

New Class: Ring Closing Vertex #53

einarkjellback opened this issue Apr 22, 2021 · 1 comment
Labels
Refactoring Fix typos, clean up code, change design

Comments

@einarkjellback
Copy link
Contributor

In making a unit test I discovered a bug that assumed Ring Closing Vertices (RCVs) are not at the scaffold level. After some discussion with @marco-foscato I was made aware that the content of RCVs are not part of the final molecule, i.e. they do not contain real atoms or molecules, so it doesn't make sense to place them at the scaffold level. Furthermore, all Rings have a head and tail vertex and these are assumed to be RCVs, but this is not explicitly checked, so programmers are not prevented from breaking this assumption. No doubt this is a slippery slope and will surely produce subtle bugs like the one I found in the future that may be hard to detect. I'm sure there are other places in the code where vertices are assumed to be RCVs.

We should solve this issue by making the assumption that a vertex is an RCV explicit where appropriate to prevent programmers from breaking this constraint.

Right now an RCV is represented as a normal Fragment with a dummy atom inside and exactly*1 attachment point (AP). This AP has an APClass which signifies that it is a Ring Closing Attractor (RCA). The APClass can be one of three choices: ATPlus, ATMinus, and ATNeutral.

I suggest we make RCV its own class called "RingCloser" which inherits from DENOPTIMVertex. We should also make a separate Enum called "Attractor" which contain the three choices discussed above. Getting rid of the dummy atom may be difficult as it relates to issue #49.
After that, the next step will be to require RingClosers where the code assumes so. A good place to start can be to change the type of the head and tail vertex in the DENOPTIMRing class from DENOPTIMVertex to RingCloser.

@einarkjellback einarkjellback added the Refactoring Fix typos, clean up code, change design label Apr 22, 2021
@marco-foscato
Copy link
Member

Having RCVs as a new class is fine, but the dummy atom MUST remain (BTW: this is totally unrelated to #49)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Fix typos, clean up code, change design
Projects
None yet
Development

No branches or pull requests

2 participants