Skip to content

#1256 - Inability to save a structure with R-Group in the SDF file format#3763

Open
ost2025 wants to merge 13 commits into
epam:masterfrom
ost2025:bug_fix_1256
Open

#1256 - Inability to save a structure with R-Group in the SDF file format#3763
ost2025 wants to merge 13 commits into
epam:masterfrom
ost2025:bug_fix_1256

Conversation

@ost2025

@ost2025 ost2025 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Generic request

  • PR name follows the pattern #1234 – issue name
  • branch name does not contain '#'
  • base branch (master or release/xx) is correct
  • PR is linked with the issue
  • task status changed to "Code review"
  • code follows product standards
  • regression tests updated

@ost2025 ost2025 self-assigned this Jul 2, 2026
@ost2025 ost2025 linked an issue Jul 2, 2026 that may be closed by this pull request
@ost2025 ost2025 marked this pull request as ready for review July 6, 2026 18:23
Comment thread api/c/indigo/src/indigo_savers.cpp Outdated
// separate sub-molecule whose atoms are not vertices of the main graph, so the
// component iteration never visits them on its own. Returns the number of such
// out-of-graph atoms (currently those held by R-group fragments).
static int countOutOfGraphAtoms(BaseMolecule& mol)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it seems like this function just counts number of all atoms in r-groups

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

logic updated

Comment thread api/c/indigo/src/indigo_savers.cpp Outdated
// atoms remain that were not part of any written fragment (e.g. a free R-group
// whose atoms live outside the main graph), emit the whole molecule as one more
// record so the owning structure is preserved instead of being lost. (#1256)
if (written_atoms == 0 && countOutOfGraphAtoms(mol) > 0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it will make more sense to write each r-group independent if it is does not belong to any existing structure. also, it makes sense to consider not only written_atoms=0 case (e.g. if structure contains r-group assigned to atoms, and r-group outside the graph

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

logic updated

root = joinPathPy("molecules/", __file__)
ref_path = joinPathPy("ref/", __file__)

files = ["1256-ketR20-from2815"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

add tests with
2 independent r-groups
1 standard r-group and 2 independent r-group
reaction with 1 and 2 independent r-groups

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Could you please provide specific files in ket format to fill the gap?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Some similar data has been added

root = joinPathPy("molecules/", __file__)
ref_path = joinPathPy("ref/", __file__)

files = ["1256-ketR20-from2815"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On top of the cases already requested above, please also add a case with ≥2 disconnected normal components plus one attached R-group (e.g. a simple salt)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Could you please provide specific files in ket format to fill the gap?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Some similar data has been added

Comment thread api/c/indigo/indigo.h Outdated
// Splits the item into fragments (connected components for a molecule,
// constituent molecules for a reaction) and returns the whole multi-record
// SDF as a string. Single entry point for SDF-by-fragments serialization.
CEXPORT const char* indigoGetFragmentSdf(int item);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Minor: Consider align naming to support consistency across application.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

naming updated

return IndigoLib.checkResultString(self._lib().indigoCml(self.id))

def getFragmentSdf(self):
"""Structure method returns the structure as a string in SDF format,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocker: Let's either add comment in all wrappers or take it off from all places.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it is depended from file. indigo_object.py has comments in this style.

- new test data added;
- r-groups processing updated;
- API renamed for naming consistency;
- comments fixed;
Comment thread api/c/indigo/src/indigo_savers.cpp Outdated
- eliminated unnecessary data copying due to the use of the clone() function;
Comment thread api/c/indigo/src/indigo_savers.cpp Outdated
Comment thread api/tests/integration/tests/formats/ket_to_sdf_1256.py
Comment thread api/c/indigo/src/indigo_savers.cpp Outdated
- copying only the one R-group we're keeping;
- test input data extended with one of the QueryMolecule case;
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.

Inability to save a structure with R-Group in the SDF file format

3 participants