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

Subgraph rewrite #878

Merged
merged 9 commits into from
Apr 30, 2022
Merged

Subgraph rewrite #878

merged 9 commits into from
Apr 30, 2022

Conversation

aerkiaga
Copy link
Collaborator

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.


The algorithm is simple: when a new atom is added, it's not assigned a subgraph quite yet. When two atoms are bonded, their subgraphs, if present, are merged. When a bond is broken, the subgraph it's in is marked dirty.

Now, when any information about subgraphs is queried, it will be returned if it refers to a clean subgraph. If it refers to a dirty subgraph, all its formed vertices are traversed and new subgraphs split from it. If it refers to a lone vertex, it will be assigned a subgraph.

This way, expensive operations are only performed once, or not at all, and any overhead is avoided.

Stable enough to not crash, but cartoons don't work yet

Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
@github-actions
Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

@aerkiaga aerkiaga marked this pull request as draft April 29, 2022 07:52
@aerkiaga
Copy link
Collaborator Author

Still WIP, as lone vertices still need to be computed when querying all subgraphs.

Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
@aerkiaga aerkiaga marked this pull request as ready for review April 29, 2022 12:51
@github-actions
Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Ubuntu-2004.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

Copy link
Member

@ghutchis ghutchis left a comment

Choose a reason for hiding this comment

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

Mostly just asking for some doc strings.

avogadro/core/graph.h Show resolved Hide resolved
avogadro/core/graph.h Outdated Show resolved Hide resolved
Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
@github-actions
Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Ubuntu-2004.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

@ghutchis
Copy link
Member

There's something else .. here's a test file:
test.cjson.zip

If you double-click with the select tool, you can select the water ligands. I deleted a few.

Then I broke the graph by deleting a carbon atom in the middle and tried to double-click one of the fragments. It hung.

I don't have time to investigate further tonight, but wanted to report the bug - and then the need to add a unit test like the previous ConnectedGraph one.

@aerkiaga
Copy link
Collaborator Author

I can try to fix it now. And regarding the testcase, I wanted to try to port the original testcase.

Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
Signed-off-by: Aritz Erkiaga <aerkiaga3@gmail.com>
@aerkiaga
Copy link
Collaborator Author

A little minimal, maybe. Should I add more testcase code?

@github-actions
Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Ubuntu-2004.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

@ghutchis
Copy link
Member

In my testing so far, this seems robust.

@ghutchis
Copy link
Member

I think for now, the test case is okay. I'll think through some more torture cases (e.g., patches of graphene)

@ghutchis ghutchis merged commit 5d03261 into OpenChemistry:master Apr 30, 2022
@aerkiaga aerkiaga deleted the subgraph-rewrite branch May 1, 2022 10:12
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