JuliaMath / InverseFunctions.jl

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

Add setinverse #25

Closed oschulz closed 2 years ago

oschulz commented 2 years ago

Closes #18

CC @cscherrer

oschulz commented 2 years ago

@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[bot] commented 2 years ago

Codecov Report

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

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 ```diff @@ Coverage Diff @@ ## master #25 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 3 4 +1 Lines 74 80 +6 ========================================= + Hits 74 80 +6 ``` | [Impacted Files](https://codecov.io/gh/JuliaMath/InverseFunctions.jl/pull/25?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath) | Coverage Δ | | |---|---|---| | [src/setinverse.jl](https://codecov.io/gh/JuliaMath/InverseFunctions.jl/pull/25/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath#diff-c3JjL3NldGludmVyc2Uuamw=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath)

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

oschulz commented 2 years ago

Thanks for the corrections, @cscherrer !

oschulz commented 2 years ago

Ok, docgen is fixed too now.

aplavin commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

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

oschulz commented 2 years ago

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

oschulz commented 2 years ago

Thanks, I'll merge then?