search

MDAnalysis / mdanalysis

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

Guesser checks should wrap NoDataError #4749

Closed IAlibay closed 3 weeks ago

IAlibay commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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).

IAlibay commented 1 month ago

@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 commented 1 month ago

I’m not following your reasoning here — could you please elaborate? The errors for missing attributes should be NoDataErrors. This subclasses AttributeError, which is probably why the previous logic worked. What’s the benefit of catching extra AttributeErrors?On Oct 20, 2024, at 22:50, Irfan Alibay @.***> wrote: @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.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>