Add setinverse #25

oschulz · 2022-09-23T14:36:18Z

Closes #18

oschulz · 2022-09-23T14:37:43Z

@devmotion, I couldn't come up with a better function name than setinverse - but maybe it's Ok since we also have things like a non-mutating setfield in other packages?

codecov · 2022-09-23T14:40:01Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (62bca9b) compared to base (34c1adf).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##            master       #25   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         4    +1     
  Lines           74        80    +6     
=========================================
+ Hits            74        80    +6

Impacted Files	Coverage Δ
src/setinverse.jl	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

cscherrer

Looking good! I had two very minor nit-picks :)

src/setinverse.jl

Co-authored-by: Chad Scherrer <[email protected]>

oschulz · 2022-09-23T14:43:04Z

Thanks for the corrections, @cscherrer !

oschulz · 2022-09-23T14:57:42Z

Ok, docgen is fixed too now.

devmotion · 2022-09-23T20:32:26Z

docs/src/api.md


 ```@docs
 InverseFunctions.square
+InverseFunctions.FunctionWithInverse


Since it's not exported and not intended to be constructed directly, I wonder if it should be omitted from the docs?

I thought people might want to have the option to dispatch on it, in low-level uses cases and so on, that's why I made it party of the official API.

I assumed that this was the motivation. I wonder, however, how often one would actually want to dispatch on FunctionWithInverse. I assume for many cases it would be simpler to just use unwrap_f (or something equivalent of the same name) and inverse - similar to the setinverse case in this PR.

I wonder, however, how often one would actually want to dispatch on FunctionWithInverse.

We may actually have occasion to do that in the pushforward measure in MeasureBase (in the context of JuliaMath/MeasureBase.jl#89, since PushforwardMeasure stores both forward and inverse function for performance reasons). Not sure yet - but it's an interesting option (we may want to extract forward and inverse from FunctionWithInverse). So there's at least one possible use case, so there may be more. And it doesn't cost us anything, I wouldn't expect FunctionWithInverse to change in breaking ways in the future.

since PushforwardMeasure stores both forward and inverse function for performance reasons

To obtain these you don't have to specialize on FunctionWithInverse though. You could just store _unwrap_f(f) and inverse(f) if an arbitrary f is provided.

src/setinverse.jl

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

src/setinverse.jl

devmotion · 2022-09-24T20:32:17Z

docs/src/api.md


 ```@docs
 InverseFunctions.square
+InverseFunctions.FunctionWithInverse


I assumed that this was the motivation. I wonder, however, how often one would actually want to dispatch on FunctionWithInverse. I assume for many cases it would be simpler to just use unwrap_f (or something equivalent of the same name) and inverse - similar to the setinverse case in this PR.

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

aplavin · 2022-09-25T07:56:55Z

I couldn't come up with a better function name than setinverse - but maybe it's Ok since we also have things like a non-mutating setfield in other packages?

Maybe, withinverse discussed in the linked issue would be more intuitive?
set... functions in packages like Accessors and ConstructionBase tend to mean "set an already existing part to a new value", not "add/insert a new part to the object".

Just a suggestion.

oschulz · 2022-09-26T08:49:57Z

Maybe, withinverse discussed in the linked issue would be more intuitive?

It would, in principle, and was my first idea for naming this functionality. However, we use with... with very different semantics in ChangesOfVariables.jl, which will often be used in conjunction with InverseFunctions.jl. Having these different with...-functions closely together in code could be quite confusing to the reader.

How about addinverse instead of setinverse?

aplavin · 2022-09-26T09:59:16Z

we use with... with very different semantics in ChangesOfVariables.jl

Interesting, if I understand correctly that package uses basically the opposite "direction" of with :)

How about addinverse instead of setinverse?

Well, with would also be consistent with FunctionWithInverse defined here...
I don't have a very strong preference here, choosing the right and general verb for such operations isn't obvious.

For example, Accessors uses "set" for setting something that already exists, and "insert" for adding a new component. Maybe, set isn't the worst choice here in the end?..
Btw, after this PR Accessors can define its set() method so that sin_with_inverse = @set inverse(sin) = asin works. Accessors already depends on InverseFunctions anyways. This is more obvious than sin_with_inverse = setinverse(sin, asin) in determining which argument is the function and which is the inverse.

cscherrer · 2022-09-26T12:48:17Z

Interesting, if I understand correctly that package uses basically the opposite "direction" of with :)

FWIW, Zygote has something similar:

help?> Zygote.withgradient
  withgradient(f, args...)
  withgradient(f, ::Params)

  Returns both the value of the function and the gradient, as a named tuple.

  julia> y, ∇ = withgradient(/, 1, 2)
  (val = 0.5, grad = (0.5, -0.25))
  
  julia> ∇ == gradient(/, 1, 2)
  true

oschulz · 2022-09-26T13:46:11Z

FWIW, Zygote has something similar ... withgradient(f, args...)

Yes, that matches what ChangesOfVariables.with_logabsdet_jacobian(f, x) does in regard to "with" - it take the same inputs as f(x) but generates additional output. Whereas setinverse takes two functions and returns a "decorated" single function - that's why I'm hesitant to use withinverse for it.

oschulz · 2022-09-26T13:47:53Z

Btw, after this PR Accessors can define its set() method so that sin_with_inverse = @set inverse(sin) = asin works.

Oh, that's very neat! Maybe setinverse isn't that bad as a function name here then? Better than addinverse at least, right?

aplavin · 2022-09-26T15:53:56Z

Yes, maybe setinverse is actually fine.

Seems like with... can indeed be understood in the "opposite" way. I've only seen Base.withenv and MonteCarloMeasurements.with_nominal before, and they are more like withinverse(sin, asin) than the other meaning.

oschulz · 2022-09-26T18:25:39Z

Seems like with... can indeed be understood in the "opposite" way.

Right, like in #6 (thanks for the reminder there, @aplavin !).

oschulz · 2022-09-27T12:25:44Z

Are you happy with how this looks now, @devmotion ?

devmotion

Looks good to me, I don't think the discussion in https://github.com/JuliaMath/InverseFunctions.jl/pull/25/files#r979044891 is blocking this PR.

oschulz · 2022-09-27T12:35:55Z

Thanks, I'll merge then?

Add setinverse

e6826aa

oschulz requested a review from devmotion September 23, 2022 14:36

cscherrer approved these changes Sep 23, 2022

View reviewed changes

src/setinverse.jl Outdated Show resolved Hide resolved

src/setinverse.jl Outdated Show resolved Hide resolved

oschulz and others added 2 commits September 23, 2022 16:42

Fix docstring of setinverse

c84aa36

Co-authored-by: Chad Scherrer <[email protected]>

Fix docstring of setinverse

f3c7053

Co-authored-by: Chad Scherrer <[email protected]>

Fix docs gen

c4c2c83

devmotion reviewed Sep 23, 2022

View reviewed changes

Improve setinverse docs and implementation

18051bd

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

devmotion reviewed Sep 24, 2022

View reviewed changes

Shorten definition of setinverse

62bca9b

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

aplavin mentioned this pull request Sep 26, 2022

Add with_inverse(f, x)? #6

Closed

oschulz mentioned this pull request Sep 26, 2022

fix invpow and invlog domains #23

Merged

devmotion approved these changes Sep 27, 2022

View reviewed changes

oschulz merged commit dd6e408 into master Sep 28, 2022

oschulz deleted the setinverse branch September 29, 2022 14:07

devmotion mentioned this pull request Oct 16, 2022

with_logabsdet_jacobian(::InverseFunctions.FunctionWithInverse, x) JuliaMath/ChangesOfVariables.jl#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setinverse #25

Add setinverse #25

oschulz commented Sep 23, 2022

oschulz commented Sep 23, 2022

codecov bot commented Sep 23, 2022 •

edited

Loading

cscherrer left a comment

oschulz commented Sep 23, 2022

oschulz commented Sep 23, 2022

devmotion Sep 23, 2022

oschulz Sep 24, 2022

devmotion Sep 24, 2022

oschulz Sep 25, 2022 •

edited

Loading

devmotion Sep 26, 2022

devmotion Sep 24, 2022

aplavin commented Sep 25, 2022 •

edited

Loading

oschulz commented Sep 26, 2022

aplavin commented Sep 26, 2022 •

edited

Loading

cscherrer commented Sep 26, 2022

oschulz commented Sep 26, 2022

oschulz commented Sep 26, 2022

aplavin commented Sep 26, 2022 •

edited

Loading

oschulz commented Sep 26, 2022

oschulz commented Sep 27, 2022

devmotion left a comment

oschulz commented Sep 27, 2022

Add setinverse #25

Add setinverse #25

Conversation

oschulz commented Sep 23, 2022

oschulz commented Sep 23, 2022

codecov bot commented Sep 23, 2022 • edited Loading

Codecov Report

cscherrer left a comment

Choose a reason for hiding this comment

oschulz commented Sep 23, 2022

oschulz commented Sep 23, 2022

devmotion Sep 23, 2022

Choose a reason for hiding this comment

oschulz Sep 24, 2022

Choose a reason for hiding this comment

devmotion Sep 24, 2022

Choose a reason for hiding this comment

oschulz Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

devmotion Sep 26, 2022

Choose a reason for hiding this comment

devmotion Sep 24, 2022

Choose a reason for hiding this comment

aplavin commented Sep 25, 2022 • edited Loading

oschulz commented Sep 26, 2022

aplavin commented Sep 26, 2022 • edited Loading

cscherrer commented Sep 26, 2022

oschulz commented Sep 26, 2022

oschulz commented Sep 26, 2022

aplavin commented Sep 26, 2022 • edited Loading

oschulz commented Sep 26, 2022

oschulz commented Sep 27, 2022

devmotion left a comment

Choose a reason for hiding this comment

oschulz commented Sep 27, 2022

codecov bot commented Sep 23, 2022 •

edited

Loading

oschulz Sep 25, 2022 •

edited

Loading

aplavin commented Sep 25, 2022 •

edited

Loading

aplavin commented Sep 26, 2022 •

edited

Loading

aplavin commented Sep 26, 2022 •

edited

Loading