JuliaMath / InverseFunctions.jl

Interface for function inversion in Julia
Other
29 stars 8 forks source link

consistently use NoInverse instead of exceptions #40

Closed aplavin closed 9 months ago

aplavin commented 10 months ago

Currently, some inverse(non-invertible f) return NoInverse, some throw an exception. (the latter is code from my earlier PRs, sorry for that!) Here, I make it consistent by always returning NoInverse.

codecov[bot] commented 10 months ago

Codecov Report

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

Comparison is base (d397fef) 100.00% compared to head (7856164) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #40 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 5 5 Lines 121 121 ========================================= Hits 121 121 ```

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

oschulz commented 10 months ago

Hm, one the one hand, this makes things more consistent. But it does introduce type instabilities, and the functions in question may well be used in time-critical code.

I would suggest to keep throwing an exception here for practical reasons. True, we'll then get a failure on certain single points, e.g. at zero. But on the other hand, if an application encounters a zero in such places, it's very likely that something is wrong, and throwing an exception would make sense.

The idea behind NoInverse is to enable code to check if a function is invertible, and then take a different code paths if it's not. But I don't think this should come at the cost of type instability.

aplavin commented 9 months ago

Yeah, tend to agree with the type stability concerns! I didn't manage to write any small self-contained benchmark that demonstrates any performance difference between NoInverse vs exception, but agree that type stability should be kept whenever possible.