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.

Sign up for GitHub

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

Jump to bottom

Addition of Orca dtype #186

Merged
merged 6 commits into from
Jun 25, 2020
Merged

Addition of Orca dtype #186

merged 6 commits into from
Jun 25, 2020

Conversation

muammar
Copy link
Contributor

@muammar muammar commented Jan 9, 2020 •
edited
Loading

Description

Orca dtype addition needed along with MolSSI/QCEngine#178

Changelog description

  • Addition of Orca `dtype.

Status

  • Code base linted
  • Ready to go

@muammar muammar mentioned this pull request Jan 9, 2020
Open
2 tasks
loriab
loriab reviewed Jan 9, 2020
View reviewed changes
qcelemental/molparse/to_string.py Outdated Show resolved Hide resolved
qcelemental/molparse/to_string.py Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jan 9, 2020

This pull request introduces 2 alerts when merging 213f285 into 178e97b - view on LGTM.com

new alerts:

  • 2 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jan 9, 2020

This pull request introduces 1 alert when merging 17b4d68 into 178e97b - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 7, 2020

This pull request introduces 1 alert when merging 33e780b into a47be4a - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@muammar muammar changed the title WIP: Addition Orca dtype Addition Orca dtype Feb 7, 2020
@muammar muammar changed the title Addition Orca dtype Addition of Orca dtype Feb 7, 2020
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 7, 2020

This pull request introduces 1 alert when merging 3e57a42 into a47be4a - view on LGTM.com

new alerts:

  • 1 for Unused local variable

dgasmith
dgasmith reviewed Feb 7, 2020
View reviewed changes
Copy link
Collaborator

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

Can you rebase this and take care of the LGTM complaint?

qcelemental/tests/test_molparse_to_string.py Show resolved Hide resolved
loriab
loriab reviewed Feb 13, 2020
View reviewed changes
qcelemental/molparse/to_string.py Outdated
@@ -136,6 +138,26 @@ def to_dict(self) -> Dict:

smol.extend(atoms)

if dtype == "orca":
Copy link
Collaborator

Choose a reason for hiding this comment

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

elif here will fix some tests.

note that if you conda install pytest, you can pytest qcelemental/ -rws -v in the main clone directory to test locally.

not mandatory, but it'd be nice to test the ghost atoms, too. there's a test you can mirror.

@codecov
Copy link

codecov bot commented Jun 24, 2020 •
edited
Loading

Codecov Report

Merging #186 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@loriab
Copy link
Collaborator

loriab commented Jun 24, 2020

Hi @muammar, I edited your branch a bit to get the orca dtype into the qcel release this week. Note that to resolve conflicts I had to force-push it, so don't try to update your local branch with pull. Let me know if you disagree with any of my changes.

loriab
loriab approved these changes Jun 25, 2020
View reviewed changes
Copy link
Collaborator

@loriab loriab left a comment

Choose a reason for hiding this comment

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

thanks for the new output type!

@loriab loriab merged commit 7f65188 into MolSSI:master Jun 25, 2020
@muammar
Copy link
Contributor Author

muammar commented Jun 25, 2020

Hi @muammar, I edited your branch a bit to get the orca dtype into the qcel release this week. Note that to resolve conflicts I had to force-push it, so don't try to update your local branch with pull. Let me know if you disagree with any of my changes.

Thanks for merging this into master @loriab !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgasmith dgasmith dgasmith left review comments

@loriab loriab loriab approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@muammar @loriab @dgasmith