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

#2748 Substituents are displayed backwards if appearing on the left of the molecule #2753

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jblack-mestre
Copy link
Contributor

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

@jblack-mestre
Copy link
Contributor Author

I have a problem with the regression tests: On my machine, my new test passes even when the output and the reference pngs are different. I have uploaded the old (incorrect) png to the ref directory - so the cd/ci tests should fail. If they don't fail, then there may be a problem when comparing the rendered pngs for my test.

@jblack-mestre
Copy link
Contributor Author

There is something wrong with the regression test, the output (correct) is:
{C03F0554-05F2-4C60-9EDA-B7DA53BBC4B5}

whereas the reference (incorrect) is:
{DA58C944-818F-4989-A165-4A5A15D907A3}

It is not failing when the SiEt3 becomes Et3Si. Have you seen this problem before or am I doing something wrong?

@@ -3477,6 +3482,42 @@ void MoleculeRenderInternal::_prepareLabelText(int aid)
}
}

void MoleculeRenderInternal::_reverseLabelText(const int aid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this function - it is too simple and will corrupt correct names.
e.g. N,N-dimethylaniline will be changed to N-dimethylanilineN,, tBu to But, DIPEA to AEPID

Copy link
Contributor Author

@jblack-mestre jblack-mestre Feb 5, 2025

Choose a reason for hiding this comment

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

Thanks for pointing that out, you are correct. Looking at chemdraw, it does reasonably well but gets DIPEA and N,N-dimethylaniline wrong
{9ADB7E41-659D-426B-8734-51575ADCAFFC}

I guess they have a library of common names and they break the substituent string into these fragments. So we would have to do something similar and implement a larger library. I'm checking on our side if we want to push forward with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chemdraw: File -> List Nicknames

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.

Substituents are displayed backwards if appearing on the left of the molecule
2 participants