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

Remove support for ts as an input to the ParmedConverter #3031

Closed
IAlibay opened this issue Nov 18, 2020 · 10 comments · Fixed by #3240
Closed

Remove support for ts as an input to the ParmedConverter #3031

IAlibay opened this issue Nov 18, 2020 · 10 comments · Fixed by #3240

Comments

@IAlibay
Copy link
Member

IAlibay commented Nov 18, 2020

Context

As discussed in #3029 (comment) the ParmedConverter currently takes either an atomgroup/universe or timestep as an input. However as of #2754, support for passing timestep objects to writers was removed.

Given this, it might make sense to remove support for ts?

Maybe we should deprecate for 1.0.x and remove for 2.0?

@avitomar12
Copy link

Hey @IAlibay Is it possible to give reference to the code for this issue?

@IAlibay
Copy link
Member Author

IAlibay commented Mar 16, 2021

@avitomar12 we are referring to:

class ParmEdConverter(base.ConverterBase):

Particularly the class' ability to take timestep objects as an input.

@lilyminium should we try to get the deprecation warning through in 1.1.0 for this?

@IAlibay IAlibay added this to the 1.1.0 milestone Mar 16, 2021
@lilyminium
Copy link
Member

I don't know how many people actually use this code instead of just the convert_to function. Did we have a deprecation warning when we removed ts from the other writers?

@IAlibay
Copy link
Member Author

IAlibay commented Mar 16, 2021

Yup:

if isinstance(obj, Timestep):
warnings.warn(
'Passing a Timestep to write is deprecated, '
'and will be removed in 2.0; '
'use either an AtomGroup or Universe',
DeprecationWarning)

@lilyminium
Copy link
Member

lilyminium commented Mar 16, 2021

Okies, sounds good. I'll do that and the chainID deprecation.

Edit: I think a PR for this issue would be for 2.0 though right

@IAlibay IAlibay modified the milestones: 1.1.0, 2.0 Mar 16, 2021
@IAlibay IAlibay mentioned this issue Mar 16, 2021
9 tasks
@kitsiosvas
Copy link

Hi, @IAlibay is this issue taken? I would like to work on this if not.

@IAlibay
Copy link
Member Author

IAlibay commented Mar 26, 2021

@kitsiosvas as far as I am aware there are no PRs open on for this issue. Whilst @avitomar12 indicated they may want to work on it ten days ago, MDAnalysis has decided to take a PR-first approach (i.e. the first PR open is the one we will review, see: https://github.com/MDAnalysis/mdanalysis/wiki/GSoC-FAQ#can-i-claim-an-issue-to-work-on). So please do feel free to open a PR addressing the issue.

IAlibay pushed a commit that referenced this issue Apr 3, 2021
Towards #3031 

# Work done in this PR
* Removes the non-working code path for timestep handling in the ParmedConverter
@IAlibay
Copy link
Member Author

IAlibay commented Apr 6, 2021

This was taken care of for 1.1. Needs forward porting.

@kitsiosvas
Copy link

@IAlibay Apologies for my delay on working for this issue. I want to inform you that I will not be able to work on this. If anyone else wants to take it, I haven't opened a PR yet so it's free. Again, apologies.

@IAlibay
Copy link
Member Author

IAlibay commented Apr 6, 2021

@IAlibay Apologies for my delay on working for this issue. I want to inform you that I will not be able to work on this. If anyone else wants to take it, I haven't opened a PR yet so it's free. Again, apologies.

No worries, thanks for letting us know :)

lilyminium added a commit to lilyminium/mdanalysis that referenced this issue Apr 28, 2021
jbarnoud added a commit that referenced this issue May 3, 2021
* 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 <jonathan@barnoud.net>

* tighten up base class checking

* update docstring

* suppres warnings

* Use absolute file paths in ITPParser (#3108)

Fixes #3037

Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>

* 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 <jonathan@barnoud.net>
Co-authored-by: Aditya Kamath <48089312+aditya-kamath@users.noreply.github.com>
Co-authored-by: Cédric Bouysset <bouysset.cedric@gmail.com>
Co-authored-by: Tyler Reddy <tyler.je.reddy@gmail.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants