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

Cannot make sample molecules from all atom types #2440

Closed
rwest opened this issue May 17, 2023 · 2 comments
Closed

Cannot make sample molecules from all atom types #2440

rwest opened this issue May 17, 2023 · 2 comments

Comments

@rwest
Copy link
Member

rwest commented May 17, 2023

Bug Description

Used for testing the database, there is a function that makes a sample molecule from a given functional group definition: rmgpy.molecule.Group.make_sample_molecule().

If you give it the simplest possible group definition for an atomtype, it should be able to make a molecule that matches.
Eg. give it the group 1 C ux and it'll return methane (the simplest way to get C). Give it 1 Cd ux and it'll give ethene (the simplest way to get C with a double bond). etc.
But there are a few atom types for which it does not work.

How To Reproduce

Introduced in commit 15ca27e in pull request #1661
is a unit test that checks them all. Basically,

        for name, atom_type in rmgpy.molecule.atomtype.ATOMTYPES.items():
             adjlist = f"1 {name} ux"
             group = rmgpy.molecule.Group().from_adjacency_list(adjlist)
             try:
                 result = group.make_sample_molecule()
                 if not result.is_subgraph_isomorphic(group):
                     failed.append(name)

And some fail.
The atom types Xo, Cq, Sib, Sibf, Siq do not make sample molecules that match their chosen atomtypes.
The atom types O4b and S4b crash when you try to make sample molecules.

Installation Information

Describe your installation method and system information.

  • true on MacOS and the ubuntu used to run the automated tests.
  • RMG version information:

Additional Context

I think the method is only used for testing the database, and is not causing any crashes. So possibly low priority. Until, however, it becomes a problem. For example, it was an atom type error (that this test would have caught) that caused a hard-to-debug segfault when updating the database a few years ago #1656
So if anyone tries adding Cq, Sib, Sibf, or Siq atoms (or O4b or S4b) to the database, they may wish this worked better.

@github-actions
Copy link

This issue is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant issue, otherwise it will automatically be closed in 30 days.

@github-actions github-actions bot added the stale stale issue/PR as determined by actions bot label Aug 15, 2023
@JacksonBurns
Copy link
Contributor

@rwest since this is a testing enhancement that only seems to come up on the frequency of years, I'll go ahead and close this now but put it in the long-term project table as a nice to have.

@JacksonBurns JacksonBurns closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2023
@JacksonBurns JacksonBurns added Priority: Low and removed stale stale issue/PR as determined by actions bot labels Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants