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

Add atom object name on the cmd.get_model() returned model #380

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

pslacerda
Copy link
Contributor

Now cmd.get_model() returns an atom model with the object name on it. I didn't want to use the object variable because it is a built-in, however I thought that any other name would be misleading.

@JarrettSJohnson
Copy link
Member

Aside from the compilation errors, I'm wondering if this is the right approach. Wouldn't each atom's object name be duplicated (wasted space)? Would it make sense to have the name be stored per-object/model rather than per-atom?

@pslacerda
Copy link
Contributor Author

I had this doubt and thought that resn is duplicate all over atoms, specially for polymers and decided to replicate.

And I'm not sure if the same happens when the string is built from the C API, but Python's strings of the same value share the same memory space.

@pslacerda
Copy link
Contributor Author

I decided to return the object name on the atom model because it's convenient. It would be even more if it worked on iterate.

Copy link
Member

@JarrettSJohnson JarrettSJohnson left a comment

Choose a reason for hiding this comment

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

LGTM, just mostly one suggestion regarding variable usage.

I take back my comment regarding having model on the model level since I was under the impression these were exported as separate models, but this probably makes the most sense to align w/ iterate.

layer3/MoleculeExporter.cpp Outdated Show resolved Hide resolved
layer2/CoordSet.h Show resolved Hide resolved
@JarrettSJohnson JarrettSJohnson merged commit d6b4762 into schrodinger:master Aug 7, 2024
3 checks passed
@JarrettSJohnson
Copy link
Member

Thanks for the PR!

@pslacerda pslacerda deleted the feature-get_model branch August 7, 2024 19:07
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