SciML / ADTypes.jl

Repository for automatic differentiation backend types
https://sciml.github.io/ADTypes.jl/
MIT License
36 stars 11 forks source link

Add safe mode #55

Closed willtebbutt closed 2 months ago

willtebbutt commented 2 months ago

Checklist

Additional context

Tapir.jl now has a "safe mode", in which lots of additional type checking is performed, in addition to some value checks. I would like to enable it by default (I'll be adding @info calls on the Tapir end to make it clear to users that they're using safe mode). This is to ensure that users are aware of this feature, which often makes debugging a lot easier, and can pick up on issues before they propagate to some arbitrary point further on in the code.

Provided that people are happy with this, if someone could advise on the appropriate version bump for this package I would appreciate it.

gdalle commented 2 months ago

Would it make sense for it to be a type parameter with two possible values, instead of a field? In other words, does it bring any benefit to have that information statically available?

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (d749b5a) to head (00985ad).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #55 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 6 6 Lines 64 65 +1 ========================================= + Hits 64 65 +1 ``` | [Flag](https://app.codecov.io/gh/SciML/ADTypes.jl/pull/55/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SciML) | Coverage Δ | | |---|---|---| | [docs](https://app.codecov.io/gh/SciML/ADTypes.jl/pull/55/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SciML) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SciML#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

willtebbutt commented 2 months ago

Good question.

It shouldn't matter for performance, because whether or not a rule operates in safe mode is baked in when you call build_rrule (extra code is injected during codegen to check pre- and post-conditions of every call to a rule that happens inside a derived rule on both the forwards and reverse passes -- this is propagated through the entire compiled function so that everything gets checked).

On a practical level, (I think) adding type parameters would technically be breaking, so it would be good to avoid if possible.

gdalle commented 2 months ago

Okay then you can bump the minor version and I'll merge this

willtebbutt commented 2 months ago

Great, thanks!

gdalle commented 2 months ago

Tests only fail due to codecov throttling, merging now