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

Support adding R side chains to molecule #26

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

jacktday
Copy link
Contributor

Use virtual atoms as markers for side chains. This is useful for producing templates for automated side chain substitution.

TODO: generate an icon that matches the existing theme

@jacktday jacktday mentioned this pull request Aug 11, 2024
@EBjerrum
Copy link
Owner

Thanks for the PR, I'm looking forward to try it out. What icon is missing? The app uses many icons from https://icons8.com, maybe you can find something suitable there? Otherwise inkscape has come in handy for creating a SVG and exporting to PNG.

@jacktday
Copy link
Contributor Author

@EBjerrum there is a toolMenu action that numbers atoms. It should have an icon of either R#, if it is restricted to virtual atoms, or A# if it is allowed to number all atoms.

I had a quick look at icon8. I couldn't figure out how to make an icon with custom text.

@EBjerrum
Copy link
Owner

EBjerrum commented Aug 14, 2024

@jacktday I have been looking at the pull request and tested it out. Cool work. But I have some comments and suggestions. Conceptually the * or R is an atomtype, and I would like to also have it the atom drop-down, eventual with a dividier as you already have for the sidebar (it is a pseudo atom after all)

When changing the atommapnumber, it would be great if the dialogue was set with the current atommapnumber rather than default zero. Is this doable?

Smiles-copy and subsequent paste work great, but, saving as a molfile and reloading, gives a vidualization of R:# rather than R#,
looking into whats going on here, reading the molfile back gives the properties on the atom
{'dummyLabel': 'R', 'molAtomMapNumber': 1,}

where parsing the SMILES give:
{'dummyLabel': '*', 'molAtomMapNumber': 1,

The symbol from the atom, i.e. atom.GetSymbol() returns the dummy label, and this the molfile loaded R groups doesn't get custom drawn. A quick fix would be to check if atom.GetSymbol() is in ["R", "*"] or maybe use the atomnumber instead?

But a more in depth fix would be to switch to the R-group to also set the dummy-label "R" in the code and then custom parse SMILES strings to switch * to dummy_label 'R' whenever dummy_label is *. I really think the drawing should as much as possible reflect the state of the underlying graph object, so having a * atom shown as R is not following that ideal.

What do you think?

@EBjerrum
Copy link
Owner

Btw, I quickly made an Icon that sort of matches the other A+ and A- icons, so don't worry about that. They were increase and decrease font from the icons8 website and png, but I found a font that matched more or less and made a custom icon. I think we simply go for A#, as there seem to be no setting or mechanism to restrict it to R groups only.

@jacktday
Copy link
Contributor Author

@EBjerrum Thank you for your feedback and for finding that bug. It looks like rdkit automatically adds the dummy_label "R" to pseudo atoms when converting to mol without my change in the code base. So pseudo atoms and R seem synonymous. I could follow their example and just add a dummy_label to all pseudo atoms. However, I do think the subscript is much prettier.

There is also the possibility of adding this formatting to all atoms e.g.
image

@jacktday
Copy link
Contributor Author

jacktday commented Aug 17, 2024

I'm also playing around with using pseudo atoms to create templates. Which simplifies template creation and allows user-created templates. Unfortunately, I'm running into valence errors when adding bonds.

image

@EBjerrum
Copy link
Owner

@EBjerrum Thank you for your feedback and for finding that bug. It looks like rdkit automatically adds the dummy_label "R" to pseudo atoms when converting to mol without my change in the code base. So pseudo atoms and R seem synonymous.

Not entirely, from/to SMILES it uses *. So it's a bit inconsistent in the writing there.

I could follow their example and just add a dummy_label to all pseudo atoms. However, I do think the subscript is much prettier.

I don't think R subscript # is the way to go. Subscript usually denotes number of atoms in chemical notation, and it seems consensus for Rgroups are R1, R2, etc.

@EBjerrum
Copy link
Owner

I'm also playing around with using pseudo atoms to create templates. Which simplifies template creation and allows user-created templates. Unfortunately, I'm running into valence errors when adding bonds.

Not sure exactly what you are aiming at here? There is already some template addition code, but indeed it seems you need to specify what to do differently when substituting an atom, adding to a bond, and whether it's single or double bond. For some templates it gets even worse (i.e. if they are asymmetrical, or added to C-N bonds etc.), and it seems difficult to make a general solution where a user can add new templates using the gui. If you have ideas how it could be done simpler/better, it could be cool.

@EBjerrum
Copy link
Owner

Are you working on it and will update your pull request?

@jacktday
Copy link
Contributor Author

jacktday commented Sep 2, 2024

@EBjerrum Sorry for the delay. I got distracted by the reaction stuff. My reaction code seems to unstable atm. So I may put that on the back burner

@EBjerrum
Copy link
Owner

EBjerrum commented Sep 2, 2024

No worries, I fully understand that there can be other priorities. BTW, I saw you made an update with some relative imports. Please do that in another branch and pull request, it's easier to manage and selective merge, if the branches are kept to a single topic or change.

@jacktday
Copy link
Contributor Author

jacktday commented Sep 2, 2024

I've moved the relative import change to another branch.

I've also reduced the scope of the R side chain change. It no longer formats the atomMapNumber as a subscript. Instead opting to remain consistent with the formatting of other atoms e.g. R:2

@jacktday
Copy link
Contributor Author

jacktday commented Sep 2, 2024

I also have some code to automatically generate icons for molecules. Is that of interest to you?

@EBjerrum
Copy link
Owner

EBjerrum commented Sep 3, 2024

Great, I'll hopefully get time to review it soon! The use-case for the python -m, is to be able to run it from another place without installing it, or?

I'm not sure about the icon code. So yes and no. What does it do? I'm a bit splitted because I want to keep rdEditor close to its core functionality which is editing RDkit molecules and being python hackable, while at the same time also want to support new functionality. The R-group expansion is part of the edit functionality, and the imports is about how to use the package, so they are very much in-scope, whereas support for exporting images is not.
That being said I have been thinking about if we could create some kind of plugin mechanism, i.e. if the icon (and other drawing producing functionality) is in another package and that is installed, a new menu and pop-up window becomes available containing the code that can be used to produce images, icons, pngs, etc. becomes available.

@jacktday
Copy link
Contributor Author

jacktday commented Sep 5, 2024

Yeah, the use case of python -m is to allow running the code without installing it. It also allows you to have a stable version installed while you develop an unstable version. I'm unsure if there is a better way of doing this.

The icon code generates a 100x100pix transparent icons for each theme from the rdkit mol. This simplifies adding new templates. I would argue that extensible templates should be a core part of the editors functionality.

I don't have any strong opinions about plugins. I am more interested in embedding the editor into other tooling.

@EBjerrum
Copy link
Owner

EBjerrum commented Sep 8, 2024

OK, thanks for the clarification. The icon generation could then come in handy to autogenerate from the template definitions, so there it would be super cool. When you say user extensible, do you then mean from the GUI entirely or via some setup file. Currently theres a need to add a couple of smarts depending on what click action is done, so it's not as straightforward as simply drawing a structure. If you can find a way to only make one definition, it could be super cool.

@EBjerrum EBjerrum changed the base branch from master to R_side_chains September 16, 2024 12:30
@EBjerrum EBjerrum merged commit d8017b0 into EBjerrum:R_side_chains Sep 16, 2024
2 checks passed
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