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

Guesser checks should wrap NoDataError #4749

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

Guesser checks should wrap NoDataError #4749

IAlibay opened this issue Oct 20, 2024 · 5 comments · May be fixed by #4755
Assignees
Milestone

Comments

@IAlibay
Copy link
Member

IAlibay commented Oct 20, 2024

Related to #4748

In the new guesser API, we often do a check for an attribute and then except on an AttributeError. This makes for an ugly capture for users when a NoDataError gets thrown instead.

@lilyminium
Copy link
Member

What do you mean by wrap -- do you mean raising a NoDataError if guessing can't happen, or catching it instead of an AttributeError? I skimmed the MDAKits failures and didn't see anything relating to this, although could have missed something due to going fast

@IAlibay
Copy link
Member Author

IAlibay commented Oct 20, 2024

@lilyminium looking at some of the failures, MDAnalysis raises a NoDataError not an AttributeError, so the traceback tells you it encountered a NoDataError and then it encountered something else, which is confusing to users. By "wrap", I mean that the except should be looking for both error types.

@lilyminium
Copy link
Member

Sorry, it's late here, I'm not sure I'm fully understanding you -- if I rephrase, the issue is that the traceback is confusing because the Topology raises a NoDataError and the guesser is catching an AttributeError (and then raising a ValueError)? Are you suggesting just switching the guesser to catch a NoDataError? (and IMO raising a NoDataError too as would also fall within the spirit of the error).

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

IAlibay commented Oct 20, 2024

@lilyminium I'm saying we should be catching both errors.

Honestly, if this is late on your end then we'll not do the release and wait until these issues are correctly handled.

@lilyminium
Copy link
Member

lilyminium commented Oct 20, 2024 via email

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.

3 participants