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

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

Open
IAlibay opened this issue Oct 20, 2024 · 4 comments · May be fixed by #4752
Open

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

IAlibay opened this issue Oct 20, 2024 · 4 comments · May be fixed by #4752
Assignees
Labels
API Component-Topology defect deprecation Deprecated functionality to give advance warning for API changes.
Milestone

Comments

@IAlibay
Copy link
Member

IAlibay commented Oct 20, 2024

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
Copy link
Member Author

IAlibay commented Oct 20, 2024

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.

@IAlibay IAlibay linked a pull request Oct 20, 2024 that will close this issue
8 tasks
@lilyminium
Copy link
Member

+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 orbeckst added Component-Topology defect API deprecation Deprecated functionality to give advance warning for API changes. labels Oct 21, 2024
@orbeckst
Copy link
Member

orbeckst commented Oct 21, 2024

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
Copy link
Member

RMeli commented Oct 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Component-Topology defect deprecation Deprecated functionality to give advance warning for API changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants