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

Switch to OpenBabel 3 #451

Merged
merged 7 commits into from
Apr 29, 2021
Merged

Switch to OpenBabel 3 #451

merged 7 commits into from
Apr 29, 2021

Conversation

alongd
Copy link
Member

@alongd alongd commented Apr 18, 2021

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #451 (ada02ec) into master (a065172) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head ada02ec differs from pull request most recent head 14f0ff2. Consider uploading reports for the commit 14f0ff2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
- Coverage   54.55%   54.51%   -0.05%     
==========================================
  Files          39       39              
  Lines       12180    12180              
  Branches     3750     3750              
==========================================
- Hits         6645     6640       -5     
- Misses       4528     4532       +4     
- Partials     1007     1008       +1     
Impacted Files Coverage Δ
arc/species/conformers.py 73.51% <100.00%> (ø)
arc/species/converter.py 74.28% <100.00%> (-0.38%) ⬇️
arc/species/xyz_to_2d.py 43.25% <100.00%> (ø)
arc/species/species.py 68.20% <0.00%> (-0.16%) ⬇️
arc/common.py 84.07% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a065172...14f0ff2. Read the comment docs.

@alongd
Copy link
Member Author

alongd commented Apr 27, 2021

@mefuller, this PR is now ready for review, please take a look

Copy link
Member

@mefuller mefuller left a comment

Choose a reason for hiding this comment

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

Environment builds faster; uniform with RMG; updates to tests are iffy without further inspection, but assume ok for now as tests pass

@alongd
Copy link
Member Author

alongd commented Apr 29, 2021

Thanks @mefuller!
The only test modification that I wasn't 100% sure about was chiral center shifts under test_determine_chirality. All others are reasonable given the OB3 migration. I just looked at the species in the test, C(O)(S)NC=CO, and processing it in RMG gives a warning that the RDKit backend failed for InChI conversion, so it makes sense that OB was involved in its processing. Now that things make more sense in that perspective as well, I'm going to go ahead and merge this PR. Thanks for reviewing!

@alongd alongd merged commit 365035b into master Apr 29, 2021
@alongd alongd deleted the ob3 branch April 29, 2021 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants