Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fast-fix-palooza #425

Merged
merged 16 commits into from
Sep 24, 2019
Merged

Fast-fix-palooza #425

merged 16 commits into from
Sep 24, 2019

Conversation

j-wags
Copy link
Member

@j-wags j-wags commented Sep 20, 2019

Status

  • Ready for review
  • Ready for merge

@j-wags j-wags changed the title Code cleanup-palooza [WIP] Code cleanup-palooza Sep 20, 2019
@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2019

This pull request fixes 1 alert when merging 35e50b0 into bc89224 - view on LGTM.com

fixed alerts:

  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2019

This pull request introduces 1 alert and fixes 1 when merging 058b5d5 into bc89224 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for 'import *' may pollute namespace

@codecov-io
Copy link

codecov-io commented Sep 20, 2019

Codecov Report

Merging #425 into master will increase coverage by 1.53%.
The diff coverage is 53.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
+ Coverage   49.35%   50.89%   +1.53%     
==========================================
  Files          35       33       -2     
  Lines        9583     9292     -291     
==========================================
- Hits         4730     4729       -1     
+ Misses       4853     4563     -290
Impacted Files Coverage Δ
openforcefield/typing/engines/smirnoff/__init__.py 100% <ø> (ø) ⬆️
openforcefield/tests/test_forcefield.py 0% <0%> (ø) ⬆️
openforcefield/topology/molecule.py 86.22% <100%> (+0.04%) ⬆️
...enforcefield/typing/engines/smirnoff/forcefield.py 80.52% <100%> (-0.06%) ⬇️
openforcefield/utils/collections.py 96% <100%> (+0.16%) ⬆️
openforcefield/utils/toolkits.py 88.07% <100%> (+0.02%) ⬆️
...enforcefield/typing/engines/smirnoff/parameters.py 85.59% <100%> (+0.02%) ⬆️
openforcefield/utils/structure.py 51.14% <50%> (-0.09%) ⬇️

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 bc89224...5551d05. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2019

This pull request introduces 1 alert and fixes 1 when merging 7834f92 into bc89224 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for 'import *' may pollute namespace

@j-wags j-wags mentioned this pull request Sep 21, 2019
@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2019

This pull request introduces 1 alert and fixes 1 when merging 4eff2d2 into bc89224 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for 'import *' may pollute namespace

@j-wags j-wags changed the title [WIP] Code cleanup-palooza Fast-fix-palooza Sep 21, 2019
@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2019

This pull request introduces 1 alert and fixes 1 when merging e094e40 into bc89224 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2019

This pull request fixes 1 alert when merging 5551d05 into bc89224 - view on LGTM.com

fixed alerts:

  • 1 for 'import *' may pollute namespace

Copy link
Member

@jchodera jchodera left a comment

Choose a reason for hiding this comment

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

Looks good!

I am uncertain about using __reduce__ as an alternative to overriding __setstate__/__getstate__ and what that might mean for forward-compatibility of serialized versions.

If we're not 100% sure of our approach, perhaps we want to note that pickle/serialization support is still experimental and we cannot guarantee future versions of the toolkit will be able to read pickled/serialized objects from this version of the toolkit?

@j-wags
Copy link
Member Author

j-wags commented Sep 24, 2019

If we're not 100% sure of our approach, perhaps we want to note that pickle/serialization support is still experimental and we cannot guarantee future versions of the toolkit will be able to read pickled/serialized objects from this version of the toolkit?

Agreed -- added

  Note that, while XML representations of ``ForceField``s are stable and conform to the SMIRNOFF
  specification, the pickled ``ForceField``s that this functionality enables are not guaranteed
  to be compatible with future toolkit versions.

to release notes

@j-wags j-wags merged commit 23f3b70 into master Sep 24, 2019
@j-wags j-wags deleted the bugfixes branch September 24, 2019 03:44
@lgtm-com
Copy link

lgtm-com bot commented Sep 24, 2019

This pull request fixes 1 alert when merging 7f4844d into bc89224 - view on LGTM.com

fixed alerts:

  • 1 for 'import *' may pollute namespace

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
3 participants