Remove ParmEd Timestep writing "support" #3240
Conversation
|
Hello @lilyminium! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-04-23 22:46:01 UTC |
lilyminium
force-pushed
the
no-parmed-ts
branch
from
527750f to
5ab9a39
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3240 +/- ##
===========================================
+ Coverage 92.81% 92.83% +0.02%
===========================================
Files 170 170
Lines 22801 22809 +8
Branches 3239 3242 +3
===========================================
+ Hits 21162 21174 +12
+ Misses 1591 1587 -4
Partials 48 48
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
thanks @lilyminium lgtm, although two questions.
I'll let you choose whether you think it's worth addressing either :)
| @@ -173,9 +173,10 @@ def convert(self, obj): | |||
| ag_or_ts = obj.atoms | |||
| except AttributeError: | |||
| if isinstance(obj, base.Timestep): | |||
| ag_or_ts = obj.copy() | |||
| raise ValueError("Writing Timesteps to ParmEd " | |||
There was a problem hiding this comment.
Since passing timesteps never really worked, and was removed in 1.1, is the logic here really required (i.e. do we really need to specifically call out passing a timestep)?
There was a problem hiding this comment.
Probably not, I just copied over the changes from 1.1
| err = "No atoms found in obj argument" | ||
| with pytest.raises(TypeError, match=err): | ||
| c = ParmEdConverter() | ||
| c.convert("we still don't support emojis :(") |
There was a problem hiding this comment.
Do we not? I thought py3+ was UTF-8 compliant?
|
How does the changelog work here? This is a forward port of the 1.1 changes right? Are we going to add the 1.1 changelog to 2.0 or does it exist in two different branches that will never see each other? @orbeckst @richardjgowers thoughts here? |
|
The 1.1 changes need to be integrated into the 2.0 CHANGELOG once 1.1 is released. |
|
Windows' lack of support for emojis is deeply upsetting :( |
This reverts commit 8b715ee.
|
@lilyminium I'll let you deal with the changelog & merging. |
* added get_connections * modified tests for atoms.bonds/angles/dihedrals etc * modified parsers and things to use get_connections or bonds * updated CHANGELOG * pep8 * undo half of PR 3160 * add intra stuff * Update package/MDAnalysis/core/groups.py Co-authored-by: Jonathan Barnoud <[email protected]> * tighten up base class checking * update docstring * suppres warnings * Use absolute file paths in ITPParser (#3108) Fixes #3037 Co-authored-by: Lily Wang <[email protected]> * Adds aromaticity and Gasteiger charges guessers (#2926) Towards #2468 ## Work done in this PR * Add aromaticity and Gasteiger charges guessers which work via the RDKIT converter. * BLD: handle gcc on MacOS (#3234) Fixes #3109 ## Work done in this PR * gracefully handle the case where `gcc` toolchain in use on MacOS has been built from source using `clang` by `spack` (so it really is `gcc` in use, not `clang`) ## Notes * we could try to add regression testing, but a few problems: - `using_clang()` is inside `setup.py`, which probably can't be safely imported because it has unguarded statements/ code blocks that run right away - testing build issues is typically tricky with mocking, etc. (though in this case, probably just need to move `using_clang()` somewhere else and then test it against a variety of compiler metadata strings * Remove ParmEd Timestep writing "support" (#3240) Fixes #3031 * Adding py3.9 to gh actions CI matrix (#3245) * Fixes #2974 * Python 3.9 officially supported * Add Python 3.9 to testing matrix * Adds macOS CI entry, formalises 3.9 support * fix changelog * special metaclass * move function down * tidy code Co-authored-by: Jonathan Barnoud <[email protected]> Co-authored-by: Aditya Kamath <[email protected]> Co-authored-by: Cédric Bouysset <[email protected]> Co-authored-by: Tyler Reddy <[email protected]> Co-authored-by: Irfan Alibay <[email protected]>
Fixes #3031
Changes made in this Pull Request:
PR Checklist