search

MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.32k stars 652 forks source link

Revert removal of `MDAnalysis.topology.guessers` in 2.x branch #4751

Closed IAlibay closed 1 month ago

IAlibay commented 1 month ago

Related to #4748

The Guessers PR removed MDAnalysis.topology.guessers, however it is being actively used downstream, see pytim for an example of this.

Since this is a major API break, we shouldn't be including it in the 2.x release, but rather deprecate that code to be removed in 3.x.

IAlibay commented 1 month ago

cc @MDAnalysis/coredevs - it's unclear to me why we were ok with doing this API break. I remember querying it at the time but I don't remember the response I had.

lilyminium commented 1 month ago

+1 to copying over and deprecating -- sounds like the best course of action to me. IMO it would have been ideal to import the methods from the new location, but making them part of DefaultGuesser made that pretty hard.

orbeckst commented 1 month ago

it's unclear to me why we were ok with doing this API break

It must have slipped through — the PR was big. I am glad that the MDAKits registry was catching it — it's a real advantage to see immediately what effect our proposed changes have on the eco-system.

I agree with the proposed course of action + deprecation.

RMeli commented 1 month ago

Yeah, GSoC PRs are notoriously insidious to review because they are often big and drag for many months. It's expected that something slips through, and I agree with @orbeckst that is great that the MDAKits catch downstream issues immediately.

+1 for adding them back with a deprecation.