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

Use of IAtomContainer tags #49

Open
einarkjellback opened this issue Apr 9, 2021 · 3 comments
Open

Use of IAtomContainer tags #49

einarkjellback opened this issue Apr 9, 2021 · 3 comments
Labels
question Further information is requested

Comments

@einarkjellback
Copy link
Contributor

CDK's IChemObject provides a property map which can hold arbitrary information about a CDK object. DENOPTIM uses this property map to store information in the form of strings about which attachment points (APs) of a Fragment belong to which of its IAtoms, found in Fragment's IAtomContainer. This information is needed when converting a graph to an actual molecule. The information is duplicated as it is also stored in an AP's source atom-field which is really the only place it should be stored. The reason for also storing this in the property map is related to the GUI (maybe @marco-foscato can elaborate?).

There are two main disadvantages of storing the source atom information in the map:

  1. The IAtom and IAtomContainer takes on some of the responsibility which belongs to the graph, specifically APs, in integrating different Vertices into a coherent molecule. This is also related to issue Separate responsibility of Fragment and IAtomContainer #47.
  2. Increased coupling between the presentation layer and the domain layer [https://en.wikipedia.org/wiki/Business_logic#Business_logic_layer]. More specifically, the AP is stored on the IAtom as the AP would be written to the SDF files that DENOPTIM uses. Ideally the presentation layer should not dictate the design of the domain layer.

We should remove the use of the property map and provide a solution for the problem related to the GUI. Maybe @marco-foscato can provide some more concrete first steps?

@einarkjellback einarkjellback added invalid This doesn't seem right question Further information is requested labels Apr 9, 2021
@einarkjellback einarkjellback removed the invalid This doesn't seem right label Apr 9, 2021
@marco-foscato
Copy link
Member

The source atom is currently identified in the AP by using the index of such source atom in the atom list. This is simply heritage from when the fragment was an extension of the AtomContainer. The use of the property map is simply a pragmatic solution: it does the job of ensuring that some AP info travels together with an atom.
Since we foresee a future where APs may or may not have a source atom (i.e., in an EmptyVertex the source atom is simply meaningless already now), the location of the information "which atom should be involved in the bond we have to make when we use this AP" and the way to store and manipulate such information will most likely change substantially.

@marco-foscato
Copy link
Member

I'm starting to think that we could use a reference to the source atom instead of the AP filed "atomPositionNumber". This would avoid a lot of fuss in ensuring consistency between this index and the atom list, but it implies that the atom must be a vertex-specific instance.

@einarkjellback
Copy link
Contributor Author

@marco-foscato Storing an IAtom reference instead of an index makes a lot of sense as it is a natural extension to our previous "spring cleaning" where we replaced the use of indices of vertices for their references.

Also, aren't atoms already specific to the vertex instance even if we are using the index?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants