-
Notifications
You must be signed in to change notification settings - Fork 603
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
Remove openbabel dependency in qchem #2415
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #2415 +/- ##
==========================================
+ Coverage 99.43% 99.45% +0.02%
==========================================
Files 243 244 +1
Lines 18987 18971 -16
==========================================
- Hits 18879 18867 -12
+ Misses 108 104 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go.
@soranjh do you also need to remove openbabel from the various actions and docs? E.g.,
|
@@ -17,7 +17,6 @@ FROM pennylane/base:latest | |||
# Update and install Qchem | |||
RUN DEBIAN_FRONTEND="noninteractive" apt-get install tzdata # need to perform this again | |||
RUN apt-get update \ | |||
&& apt-get -y install --no-install-recommends make git openbabel \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soranjh sorry I missed this one! But openbabel
was only one of the packages being installed, do we still need make
and git
? If so, we will need to revert the line to
&& apt-get -y install --no-install-recommends make git \
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Josh. I am actually making my final PR to bring qchem inside pennylane and remove the need for installation. I can create a small PR just to retrieve this line during the few days transition period until that PR gets merged.
Context:
This PR removes the
qchem
dependency toopenbabel
which was used for converting different molecular geometry formats to thexyz
format. We can safely expect the users to perform such conversions. The PR also collects the general-purpose functions ofqml.qchem
in a newstructure
module. All the functions in the newstructure
module will be still accessible withqml.qchem.function_name
.Description of the Change:
The function
read_structure
is modified to remove its dependency toopenbabel
. The functionsread_structure
,active_space
,excitations
,hf_state
andexcitations_to_wires
are moved insidestructure
.Benefits:
Removing
qchem
dependencies.Possible Drawbacks:
Related GitHub Issues: