fix inverse(^) domain

[aplavin]

Technically breaking, but the old behavior was wrong - so this is a bug fix. Inverse to y=x^2 at y=4 isn't 2 - it's not defined.

Maybe there are better and less error-prone approaches to handling these domains?..

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.82%. Comparing base (ad56514) to head (50f3fca). Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/functions.jl	83.33%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
- Coverage   99.25%   97.82%   -1.43%     
==========================================
  Files           6        6              
  Lines         134      138       +4     
==========================================
+ Hits          133      135       +2     
- Misses          1        3       +2     

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

[oschulz]

Inverse to y=x^2 at y=4 isn't 2 - it's not defined.

Why can't it be defined? x^2 is invertible when restricted to positive values.

[aplavin]

g(y) is inverse to f(x) requires g(f(x)) == x for all x, and it seems only reasonable to require this from definitions in a generic package. You won't do inverse(sin) = asin here, right? This is exactly the same situation: sin is invertible on -pi/2..pi/2 but not on its whole range; x^2 is invertible on 0..Inf but not on its whole range.

These usecase-specific inverses are what setinverse for, imo.

[oschulz]

Yes, you do have a point @aplavin . In any case, I don't think people will often need the inverse for pow in typical function chains. The question is, it it problematic enough to warrant a breaking release?

[aplavin]

That's a bugfix, so a regular release should be fine - without changing the major version.

[oschulz]

I have no strong opinion here - @devmotion ?

[aplavin]

Also relates to https://github.com/JuliaMath/InverseFunctions.jl/issues/10.

Yeah, a nice way to support those would be nice! Personally, I have multiple usecases for inverses, and some of them in principle require different kinds of inverse.

• Computing preimage given a function y=f(x) and a set Y = y1..y2. If f⁻¹ exists and f⁻¹(y1) < f⁻¹(y2), then the preimage of Y is X = f⁻¹(y1) .. f⁻¹(y2) (reversed for >). This requires the proper inverse, as inverse() is defined to be in this package. Otherwise, for f(x) = x^2 and Y=1..4, one gets X = 1..2 which is not the correct answer. The correct one is an error: the function isn't invertible.

• Using @set from Accessors with "invertible" functions, like @set a.x^2 = 4. Here, a right inverse is enough: "set a.x to whatever value, so that a.x^2 == 4".

Given a single function inverse, it's best to go with the "proper inverse" meaning, otherwise it's possible to get silently incorrect results.

[oschulz]

So should be do a breaking release with this now, or keep it until we have to do a breaking release anyway?

[devmotion]

Depends on how urgent we want this to be released and if any other breaking changes are expected I assume :shrug: Maybe it would be good to address #10 in the same release.

[oschulz]

Yes doing something about #10 would be great - @aplavin would you like to draft something?

[aplavin]

Well, unfortunate that this change is considered breaking instead of a bugfix: changing from objectively incorrect to correct behavior. Supporting left/right inverses is orthogonal, because this PR is only concerned with the correctness of inverse.

[oschulz]

Well, unfortunate that this change is considered breaking instead of a bugfix: changing from objectively incorrect to correct behavior.

I wouldn't say the current behavior is "obviously incorrect" - it's a question of how narrow one defined invertibility (full domain or part of the domain). I agree this change should be made, the question is whether it's of any urgency, since it just removes some (probably little-used) "incorect" functionality, or whether it should wait until we have to make other breaking changes anyway. At some point, we may also want to do a v1.0 release, that would also be a good opportunity.

[ParadaCarleton]

I agree this change should be made

If that's the case, better sooner than later I think. The longer we wait the more likely it is someone will depend on this (incorrect) functionality.

[oschulz]

You make a good point @ParadaCarleton , if there are no objections (@devmotion ?), I'll merge and release this. @aplavin , should we do #38 as well then?

[aplavin]

Yes I think this should be merged, as a bugfix.

[aplavin]

I also looked at other functions and fixed their inverse domains. It's quite hard to be sure that no edgecase is missed there! (most definitely some still remain) Nice that it should only be defined in a single place in this library, users shouldn't care about these details themselves.

[oschulz]

Yes I think this should be merged, as a bugfix.

If I understood @devmotion correctly, he would prefer a breaking release. @devmotion ?

[aplavin]

Sorry, I didn't imply any specific numbering of the next release. No matter the version number, this PR is still fundamentally a fix making behavior correct, following the function semantics and docs.

Would appreciate another look at the conditions in the code, especially important if such a fix involves a "breaking" version bump.

[oschulz]

Thank let's maybe just make a v0.2 and be done with it. :-) Good from your side @devmotion ?

[aplavin]

Not that I have a lot of hope, but... Is there a chance this bugfix could be accepted anytime soon? I've stumbled across InverseFunctions silently doing a wrong thing several times over this year, would really prefer if it immediately threw an exception.

Another recent example:

julia> f = Base.Fix1(log, 1)

# would throw a DomainError with this PR:
julia> inverse(f)(f(10))
1.0

[oschulz]

Not that I have a lot of hope, but... Is there a chance this bugfix could be accepted anytime soon

My apologies, I kinda lost track of this one. Let's get it in ~(I'll add some minor change requests in a minute).~ (looks fine to me).

[oschulz]

Looking at this again, I think it's indeed a bugfix, and so non-breaking. @devmotion any objections?

[oschulz]

@aplavin could you look into adding the tests that @devmotion mentioned, before this gets merged?

[oschulz]

@devmotion : Are the errors for Base.Fix1(^ and log tested somewhere?

@aplavin is that covered already?

[aplavin]

sorry, almost forgot about it!

[oschulz]

Thanks @aplavin !

[oschulz]

@devmotion , good from your side now?

[devmotion]

There are merge conflicts?

[oschulz]

There are merge conflicts?

Oh, so there are, silly me. @aplavin , pretty please?

[aplavin]

Indeed, there were quite some code changes during this year :) Not sure what exactly went wrong here and caused this delay, even the original feature was accepted faster than the bugfix...