Improve errors #478
Improve errors #478
Conversation
|
(Let me know when you want a review btw, I'm ignoring this for now whilst it's a draft -- which I'm taking as a "you don't need to review this yet" signal) |
Yea, that's exactly what I mean by draft, you can ignore it. But with the weird backward edge case resolved, it is ready for you to take a look at |
diffrax/_integrate.py
Outdated
| if not _term_compatible( | ||
| yi, args, term, arg, term_contr_kwarg, bwd_compat | ||
| ): |
There was a problem hiding this comment.
I think this is now wrong, as the return type is a 2-tuple rather than a boolean?
I think perhaps we should instead change the name and signature of this function to be _assert_term_compatible(...) -> None, and have it raise an error if it finds an issue?
In a couple of cases (here; in the backward compatibility), we can then try-except the error in question. (Here to do a raise ValueError(...) from e; in the backward compability just to treat it as a boolean.)
There was a problem hiding this comment.
Sure, changing to assert makes sense, then I can make the try catch block explicitly around the backward compatibility code rather than as a flag. It's not hurdle keeping it, but I'm curious what's the motivation for this specific backward compatible section? Diffrax (and equinox) have introduced breaking/non backward compatible changes in other areas (I think new events did for example, and equinox is about with jax, and I'm sure other cases).
There was a problem hiding this comment.
Really just that I try to avoid backward compatibility breaks if it's reasonable to do so.
The change with events, for example, includes a similar backward compatibility converter. That one was a little imperfect because we no longer provide the same information as the old events API (which was far too broad), but was still a best-effort!
There was a problem hiding this comment.
I admire your commitment to backwards compatibility for pre 1.0 releases, in the crucible I was forged (qiskit) there were so many breaking changes in minor releases pre 1.0 that they literally made a YouTube series called “breaking changes” to help explain them all every minor release
|
Great! This LGTM. Looks like one of the tests has gone a bit wonky, I'm not sure why. Anyway, happy to merge this once things are green. (Whilst I'm here, I'm also noticing that our tests have quite a few warnings in them. Some of these are expected -- fixed upstream in Equinox, which should have a new release any day now. The one about |
|
And merged! :) |
Previously, the abstract term error, now:
#446:
ValueError: Error while tracing ODETerm(vector_field=<function equations>).vf: matmul input operand 1 must have ndim at least 1, but it has ndim 0#461:
ValueError: The brownian increment BrownianIncrement(dt=f32[], W=f32[5]) does not have the minimal Levy Area <class 'diffrax._custom_types.AbstractSpaceTimeLevyArea'>.#474:
ValueError: The brownian increment BrownianIncrement(dt=f32[], W=f32[]) does not have the minimal Levy Area <class 'diffrax._custom_types.AbstractSpaceTimeLevyArea'>.Just generally propagating the actual error