fix inverse(^) domain #34

aplavin · 2023-07-13T09:22:26Z

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 · 2023-07-13T09:26:58Z

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 ⚠️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

oschulz · 2023-07-13T11:56:19Z

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 · 2023-07-13T12:00:58Z

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 · 2023-07-13T12:04:53Z

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 · 2023-07-13T12:06:44Z

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

oschulz · 2023-07-13T12:21:15Z

I have no strong opinion here - @devmotion ?

devmotion

I have the feeling this should be addressed more generally at some point, I don't like that there's no easy way to say "give me the inverse of some function whose domain is the positive real line". Using setinverse is cumbersome and redundant in these cases, and if you want to deal with generic functions this workaround is not even possible.

For the time being, both adopting a convention (always positive real line in these cases?) and throwing an error seem fine to me. I am not sure if there are examples but I think it's not unlikely that someone relied on or assumed we had such a convention - so I would rather be more cautious here and tag a breaking release.

Also relates to #10.

src/functions.jl

aplavin · 2023-07-14T22:51:45Z

Also relates to #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 · 2023-07-15T16:20:46Z

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

devmotion · 2023-07-15T20:30:50Z

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

oschulz · 2023-07-15T20:46:37Z

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

aplavin · 2023-07-31T21:23:27Z

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 · 2023-08-01T09:54:19Z

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 · 2023-11-02T02:29:32Z

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 · 2023-11-02T08:03:17Z

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 · 2023-11-25T14:54:09Z

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

aplavin · 2023-11-25T16:22:47Z

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 · 2023-11-25T20:12:23Z

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

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

aplavin · 2023-11-26T03:11:09Z

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 · 2023-11-26T18:31:42Z

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

aplavin · 2024-08-20T18:19:25Z

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 · 2024-08-20T18:24:24Z

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 · 2024-08-20T18:31:01Z

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

src/functions.jl

src/inverse.jl

devmotion · 2024-08-20T18:47:11Z

test/test_inverse.jl

@@ -80,7 +80,9 @@ InverseFunctions.inverse(f::Bar) = Bar(inv(f.A))
    @test_throws DomainError inverse(Base.Fix2(^, 0.5))(-5)
    @test_throws DomainError inverse(Base.Fix2(^, 0.51))(complex(-5))
    InverseFunctions.test_inverse(Base.Fix2(^, 0.5), complex(-5))
-    @test_throws DomainError inverse(Base.Fix2(^, 2))(-5)
+    @test_throws DomainError inverse(Base.Fix2(^, 2))
+    @test_throws DomainError inverse(Base.Fix2(^, -4))


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

@aplavin I think there's not test for that yet, correct?

Co-authored-by: David Widmann <[email protected]>

oschulz · 2024-08-26T11:03:54Z

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

oschulz · 2024-09-13T11:57:16Z

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

@aplavin is that covered already?

aplavin · 2024-09-13T13:36:14Z

sorry, almost forgot about it!

oschulz · 2024-09-13T18:54:25Z

Thanks @aplavin !

oschulz · 2024-09-13T18:54:42Z

@devmotion , good from your side now?

devmotion · 2024-09-13T19:39:53Z

There are merge conflicts?

oschulz · 2024-09-13T19:53:42Z

There are merge conflicts?

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

aplavin · 2024-09-13T20:39:13Z

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

oschulz · 2024-09-15T08:34:02Z

Thanks for rebasing @aplavin (and sorry we've dragged our feet on this one in between). @devmotion ?

oschulz · 2024-09-15T21:12:35Z

@aplavin could you add a minor version bump? Then I'll merge and release.

aplavin · 2024-09-16T16:58:21Z

You mean patch version, right? Bumped!

oschulz · 2024-09-22T19:39:19Z

Sorry, @aplavin , forgot to trigger registrator. Just did:

JuliaRegistries/General#115708

oschulz · 2024-09-23T10:48:38Z

@devmotion do you know why TagBot keeps failing now?

devmotion · 2024-09-23T11:57:46Z

Probably JuliaRegistries/TagBot#350 which seems to be fixed in the latest release.

oschulz · 2024-09-23T11:58:55Z

Do we have to take any action, re-run it or so?

devmotion · 2024-09-23T11:59:26Z

Already done: https://github.com/JuliaMath/InverseFunctions.jl/actions/runs/10993751187/job/30520899721

oschulz · 2024-09-23T11:59:51Z

Oh, right - sorry, just saw it. Thanks!

fix inverse(^) domain

ff3852c

oschulz requested a review from devmotion July 14, 2023 12:47

devmotion reviewed Jul 14, 2023

View reviewed changes

src/functions.jl Outdated Show resolved Hide resolved

oschulz mentioned this pull request Nov 7, 2023

Partial Inverses #39

Closed

more inverse domain fixes

ba8e13d

devmotion reviewed Aug 20, 2024

View reviewed changes

aplavin and others added 2 commits August 20, 2024 19:58

Update src/functions.jl

a23b811

Co-authored-by: David Widmann <[email protected]>

Update src/functions.jl

3d4be83

Co-authored-by: David Widmann <[email protected]>

aplavin and others added 3 commits August 20, 2024 19:59

Update src/inverse.jl

7db1ae3

Co-authored-by: David Widmann <[email protected]>

Update src/inverse.jl

415d4c3

Co-authored-by: David Widmann <[email protected]>

fix

157f7f0

add tests

3f093b8

Merge branch 'master' into patch-2

504cc39

test

d99f067

devmotion approved these changes Sep 15, 2024

View reviewed changes

bump

50f3fca

oschulz merged commit beb5746 into JuliaMath:master Sep 16, 2024
6 of 9 checks passed

aplavin deleted the patch-2 branch September 24, 2024 21:30

fix inverse(^) domain #34

fix inverse(^) domain #34

Conversation

aplavin commented Jul 13, 2023 • edited Loading

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

oschulz commented Jul 13, 2023

aplavin commented Jul 13, 2023

oschulz commented Jul 13, 2023

aplavin commented Jul 13, 2023 • edited Loading

oschulz commented Jul 13, 2023

devmotion left a comment

Choose a reason for hiding this comment

aplavin commented Jul 14, 2023

oschulz commented Jul 15, 2023

devmotion commented Jul 15, 2023

oschulz commented Jul 15, 2023

aplavin commented Jul 31, 2023

oschulz commented Aug 1, 2023

ParadaCarleton commented Nov 2, 2023

oschulz commented Nov 2, 2023

aplavin commented Nov 25, 2023

aplavin commented Nov 25, 2023 • edited Loading

oschulz commented Nov 25, 2023

aplavin commented Nov 26, 2023

oschulz commented Nov 26, 2023

aplavin commented Aug 20, 2024

oschulz commented Aug 20, 2024 • edited Loading

oschulz commented Aug 20, 2024

devmotion Aug 20, 2024

Choose a reason for hiding this comment

oschulz Aug 21, 2024

Choose a reason for hiding this comment

oschulz commented Aug 26, 2024

oschulz commented Sep 13, 2024

aplavin commented Sep 13, 2024

oschulz commented Sep 13, 2024

oschulz commented Sep 13, 2024

devmotion commented Sep 13, 2024

oschulz commented Sep 13, 2024

aplavin commented Sep 13, 2024

oschulz commented Sep 15, 2024

oschulz commented Sep 15, 2024

aplavin commented Sep 16, 2024

oschulz commented Sep 22, 2024

oschulz commented Sep 23, 2024

devmotion commented Sep 23, 2024

oschulz commented Sep 23, 2024

devmotion commented Sep 23, 2024

oschulz commented Sep 23, 2024

aplavin commented Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading

aplavin commented Jul 13, 2023 •

edited

Loading

aplavin commented Nov 25, 2023 •

edited

Loading

oschulz commented Aug 20, 2024 •

edited

Loading