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

Improve CJSON readability while maintaining size #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matterhorn103
Copy link

See OpenChemistry/molecules#5 for the background.

@ghutchis
Copy link
Member

ghutchis commented Jan 6, 2025

I unfortunately introduced a merge conflict when fixing up atom order. (The acetylene and ethylene were ending up on top of the metal atoms.)

This LGTM - please add the script to the repo and consider dropping all partialCharge bits, since they are irrelevant in fragments (i.e., invalidated as soon as they are added to the molecule)

@matterhorn103
Copy link
Author

I'll sort it.

@ghutchis
Copy link
Member

ghutchis commented Jan 6, 2025

We could actually revert to your version - I fixed it in code as well (e.g., any of those haptic ligands could have trouble because the code might pick the wrong dummy atom to attach)

But it's worth stripping the partialCharge key - just wasted space. And if you want to round off the coordinates a bit to save space, I wouldn't say no. 😉

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