Skip to content

Conversation

@johnmay
Copy link
Contributor

@johnmay johnmay commented Nov 13, 2025

  • Inter fragment bonds should not be traversed
  • Emit * (ChemEl.R) atoms first, this yields prettier output and smaller CXSMILES
  • Minor optimisation, collect up the start/root atoms in the first pass for a simpler and more efficient write pass

- Inter fragment bonds should not be traversed
- Emit * (ChemEl.R) atoms first, this yields prettier output and smaller CXSMILES
- Minor optimisation, collect up the start/root atoms in the first pass for a simpler and more efficient write pass
@dan2097 dan2097 merged commit 6757e9e into dan2097:master Nov 16, 2025
4 checks passed
@dan2097
Copy link
Owner

dan2097 commented Nov 16, 2025

The SMILES writing starts off with the logic:

		List<Atom> atomList = structure.getAtomList();
		for (Atom atom : atomList) {
			atom.setProperty(Atom.VISITED, null);
		}

but this doesn't guarantee that this flag is set to null on all atoms encountered if there are multiple fragment objects. I've set it null in your check for whether the an atom belongs to another fragment:

neighbour.setProperty(Atom.VISITED, null);//Ensure this isn't visited by traverseSmiles

which I think solves it. In the original implementation for ethyl acetate asking for the SMILES of both groups would give an NPE when performing this for acetate as in some places traverseSmiles would still visit the ethyl.

There are still some quirks to the SMILESWriter if it's called on a structure with stereochemistry prior to the call to make the hydrogens explicit e.g. tyrosine, but for most structures it can now succesfully produce SMILES of the constitutent fragments which is a nice improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants