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

#2741 Image missing stereochemistry information when using abbreviations #2742

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

Is is possible to write a unit test in api/cpp/tests/rendering/basic.cpp for this bug and others? These changes affect render_internal.cpp

@AlexanderSavelyev
Copy link
Collaborator

I guess in this particular case it is not render issue. It is just missing stereocenter, which is worth to test using just stereo information (e.g. number of stereo bonds down)

@jblack-mestre
Copy link
Contributor Author

Unfortunately we can't count the up and down bonds for the unit test; in this case, indigo correctly assigns the stereocenters with the correct bond directions, but when the molecule is passed through MoleculeRenderInternal::_prepareSGroups(), the stereocenter to the super atom is forgotten.
Can you suggest another place for a unit test?

@AlexanderSavelyev
Copy link
Collaborator

for render tests we use regression test with pixel comparison
e.g. https://github.com/epam/Indigo/blob/master/api/tests/integration/tests/rendering/render_reactions.py#L108
in order to execute regression tests need to build indigo-python target

cmake --build . --config Release --target indigo-python
# copy
python -m pip install ..\dist\epam.indigo-1.29.0.dev1-py3-none-win_amd64.whl
# run tests by mask
python3 ..\api\tests\integration\test.py -p render_reactions

also, there are some requirements should be installed to python

python3 -m pip install Pillow

@AlexanderSavelyev
Copy link
Collaborator

please also merge latest changes from master branch

@jblack-mestre
Copy link
Contributor Author

jblack-mestre commented Jan 31, 2025

thanks for the info, I've added a test to render_reactions.py.

I merged the changes from master yesterday. Git merge upstream/master is telling me I am up to date

@jblack-mestre jblack-mestre force-pushed the 2741_missing_stereochemistry branch from f5d1553 to b8c8d91 Compare January 31, 2025 12:31
@jblack-mestre
Copy link
Contributor Author

Do you know what the problem is? My fork is in sync with indigo/master and my branch is up to date with my fork's master and the indigo/master branches

@AlexanderSavelyev
Copy link
Collaborator

I just verified, your branch is not up-to-date
please do
git fetch upstream
git merge upstream/master

@jblack-mestre
Copy link
Contributor Author

git_fetch

@AlexanderSavelyev
Copy link
Collaborator

oh, I see, Indigo team didn't backmerge to master the changes about github actions. Will fix it and let you know

@AlexanderSavelyev
Copy link
Collaborator

please take the latest changes from master

@jblack-mestre
Copy link
Contributor Author

I don't have access to a linx/mac machine in order to generate the reference files for the regression test. How is this normally done?

The windows tests fail due to a diff in the output - do I need to update a output reference file?

@AlexanderSavelyev
Copy link
Collaborator

  • no need to merge from master anymore. It was just one issue with checkout github action upgrade. There is no need to PR to be up-to-date with master branch (and in general it is not possible)
  • for windows it seems that the test is running ok, but the console output is different. please replace the expected output from "out" folder into "api/tests/integration/ref/rendering/render_reactions.py.out"
  • you can check if test is passed by running python3 api\tests\integration\test.py -p render_reactions locally
  • the image from win folder should be copied into linux and mac. Usually the images are the same, but if there are difference - the test will print out the expected result

@jblack-mestre
Copy link
Contributor Author

thanks for the info - I've made the changes

@AlexanderSavelyev AlexanderSavelyev merged commit 121b7d2 into epam:master Feb 4, 2025
57 checks passed
@AlexanderSavelyev
Copy link
Collaborator

all tests passed, merged, thanks so much for the fix!

@jblack-mestre jblack-mestre deleted the 2741_missing_stereochemistry branch February 7, 2025 10:38
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.

Image missing stereochemistry information when using abbreviations
2 participants