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

Accesing qc.models.Molecule.show() #294

Closed
VHchavez opened this issue Jun 16, 2022 · 7 comments
Closed

Accesing qc.models.Molecule.show() #294

VHchavez opened this issue Jun 16, 2022 · 7 comments

Comments

@VHchavez
Copy link

VHchavez commented Jun 16, 2022

Hello,
We are using Psi4 within ChemCompute at the latest MolSSI workshop in Texas. We can visualize geometries with NGLViewer using the show method of the qc.models.Molecule class. Currently, we define a function to use as a shortcut:

def molshow(psi4geo):
    molplot = qc.models.Molecule.show(psi4geo)
    return molplot

where psi4geo is a psi4.core.Molecule

A quick set of questions:

  1. Is there a better way of doing this that we are missing?
  2. How hard would it be to add this functionality to the Psi4 molecule, e.g., psi4.core.Molecule.show(). I'm uncertain of much interplay exists between the two Classes.

I'd be happy to work on a PR, but any advice would be appreciated :)
Thanks.

@loriab
Copy link
Collaborator

loriab commented Jun 16, 2022

I'm not quite following your code. What is psi4geo -- str, psi4 Mol, etc.? The below works. I don't see how you're creating the qcel.models.Molecule object in your snippet.

import qcelemental as qcel

psi4geo = """
He 0 0 0
"""

molplot = qcel.models.Molecule.from_data(psi4geo).show()

print(molplot)

@VHchavez
Copy link
Author

Ope, sorry about that. It is a psi4 geometry:

import psi4
# Using the function definition from above. 
benzene = psi4.geometry("pubchem:benzene")
molshow(benzene)

@loriab
Copy link
Collaborator

loriab commented Jun 17, 2022

Ok, at first inspection, I didn't think your molshow would work at all. Inspecting further, psi4geo is taking the argument of self, and since the only operations on self in the show fn is to_string, and psi4.core.Molecule also has a to_string, your fn works.

Therefore, I think you could make a psi4molinstance.show() work by attaching it to the class like these other mol extensions. https://github.com/psi4/psi4/blob/master/psi4/driver/molutil.py#L227 It'd be good to also have a test case in the psi4 tests and a "using" fn for nglview in /tests/pytests/addons.py

@VHchavez
Copy link
Author

Oh wow, that's pretty nifty! I've never added functions in this fashion. It works just fine 🐶.

Questions:

  1. To add the "using" test, do I need to simply add NGLview as a new key here?
  2. What would be an appropriate test? Assert that an nglview.widget.NGLWidget object gets returned?

Thanks.

@loriab
Copy link
Collaborator

loriab commented Jun 23, 2022

To add the "using" test, do I need to simply add NGLview as a new key here?

Yes, that should work fine.

What would be an appropriate test? Assert that an nglview.widget.NGLWidget object gets returned?

Agreed, just check the type returned.

@coltonbh
Copy link
Collaborator

@VHchavez has your issue been fully addressed? Can I close now? Just doing some cleanup on old issues :) Thanks!

@coltonbh
Copy link
Collaborator

@VHchavez thank you!

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

No branches or pull requests

3 participants