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.

Sign up for GitHub

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

Jump to bottom

Aromatics, round 2 #41

Merged
merged 12 commits into from
Jun 3, 2024
Merged

Aromatics, round 2 #41

merged 12 commits into from
Jun 3, 2024

Conversation

pckroon
Copy link
Owner

@pckroon pckroon commented May 14, 2024 •
edited
Loading

So far this PR enables the dekekulization of regions that show DIME.
Still TODO:

  • Implement kekulize function
  • Implement dekekulize function
  • Update README

Note that the (de)kekulize functions will remain separate from the mark_aromatic_atoms, because the latter is slightly more clever

@pckroon pckroon requested a review from fgrunewald May 14, 2024 12:34
@pckroon pckroon requested a review from fgrunewald May 21, 2024 09:18
fgrunewald
fgrunewald requested changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@fgrunewald fgrunewald left a comment

Choose a reason for hiding this comment

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

There are two test cases that seem strange to me; otherwise the code looks

tests/test_smiles_helpers.py Show resolved Hide resolved
tests/test_smiles_helpers.py Show resolved Hide resolved
@pckroon
Copy link
Owner Author

pckroon commented May 28, 2024

For both of the tests you note, the function tested is mark_aromatic_atoms, which actually doesn't adjust the bond orders (that's mark_aromatic_edges)

pckroon added 12 commits May 29, 2024 14:31
@pckroon
Make mark_aromatic_atoms more aggressive
2bb27d7 
It now de-kekulizes all regions that show DIME, and kekulizes all regions that don't.
Part of this change is the removal of the `atoms` argument to the function, and introduction of the `strict` argument.
The removal of the `atoms` argument also changed the behaviour of 2 test cases, where all non-saturated atoms would be eligible for making aromatic regions.
@pckroon
Add a deranged naphthalene as testcase for the dekekulization
e843622
@pckroon
Improve formatting of test failures
cfb5e67
@pckroon
Expand docstring of mark_aromatic_atoms
d671024
@pckroon
Implement kekulize and dekekulize
a24dcbb
@pckroon
Limit aromaticity to elements in AROMATIC_ATOMS
4b482b2
@pckroon
Expose strict option to correct_aromatic_rings
fabdd15
@pckroon
Add simple tests for (de)kekulize
1ce2960
@pckroon
Add (de)kekulize to hypothesis test
7658aa7
@pckroon
Cleanup smiles_helper
76e17e2
@pckroon
Describe new aromaticity algorithm in readme
091dca9
@pckroon
Remove incorrect "alternative" comment
131afcb
@pckroon pckroon requested a review from fgrunewald May 29, 2024 12:34
fgrunewald
fgrunewald approved these changes Jun 3, 2024
View reviewed changes
Copy link
Collaborator

@fgrunewald fgrunewald left a comment

Choose a reason for hiding this comment

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

I think this is ok, if cgsmiles fails it means our tests suck. In any case I cannot test this branch due to all the rebasing and overwrites.

@pckroon pckroon merged commit 0ee2296 into master Jun 3, 2024
6 checks passed
@pckroon pckroon deleted the aromatic_readme branch June 3, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgrunewald fgrunewald fgrunewald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pckroon @fgrunewald