Skip to content

Conversation

@tpapp
Copy link
Owner

@tpapp tpapp commented Nov 25, 2025

  • remove robust_eltype, replace with _ensure_float
  • remove Union{} as a corner case for empty (named)tuples and Constant, use Bool
  • clarify API guarantees (inverse_eltype is always <:Real)
  • remove _ensure_float from inverse! definition, clarify invariant (eltype is always the same as inverse_eltype)
  • incidental: fix various minor code redundancies
  • fixes inverse eltype calculation fails for vectors that do not contain scalars #148 (add tests)

- remove `robust_eltype`, replace with `_ensure_float`
- remove `Union{}` as a corner case for empty (named)tuples and `Constant`, use `Bool`
- clarify API guarantees (`inverse_eltype` is always `<:Real`)
- remove `_ensure_float` from `inverse!` definition, clarify invariant
  (eltype is always the same as `inverse_eltype`)
- incidental: fix various minor code redundancies
- fixes #148 (add tests)
- get rid of `promote_op`
- add test for `Any[...]` input
- enable tests which now pass (unrelated)
@tpapp
Copy link
Owner Author

tpapp commented Nov 25, 2025

@devmotion: I wonder if this addresses your use case in #132. We fall back to Bool as the narrowest type instead of Union{}, it simplifies the code greatly. The examples from #132 work:

julia> b = Bool[]
Bool[]

julia> sum(b)
0

julia> mean(b)
NaN

julia> std(b)
NaN

If this is satisfactory, and you have time, please also review.

Copy link
Collaborator

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked around this issue for now but IIRC there are still some annoying edge cases. I assume that falling back to a numeric type such as Bool (which is not as invasive as Float64) would generally make it less likely to run into such as issues as in #132. So I'm in favour of this change.

@Ickaser
Copy link
Contributor

Ickaser commented Nov 26, 2025

As the person using TransformVariables with Unitful: there are at least a couple tests that will be doing inverse transforms with Unitful in a way that resembles my use case, e.g.

t1 = TVScale(2u"m") TVShift(a) TVExp()
test_transformation(t1, y -> y > a*2u"m", jac=false)
t2 = TVScale(1u"s") TVShift(a) TVScale(b-a) TVLogistic()
test_transformation(t2, y -> (a*u"s" < y < b*u"s"), jac=false)

If you want to be extra careful, you could add an @inferred to the test_transform function, right here:

if test_inverse
x2 = inverse(t, y)
@test x x2 atol = 1e-6
ι = inverse(t)
@test x2 == ι(y)

tpapp and others added 7 commits December 1, 2025 10:23
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
@tpapp
Copy link
Owner Author

tpapp commented Dec 1, 2025

@Ickaser: thanks for the reminder. I am comitted to maintaining support to Unitful and similar approaches. At the same time, I would prefer to error on eg complex arguments that would silently creep through. I added tests for both, now Unitful will definitely work without broadening to ::Number in too many places.

@tpapp tpapp merged commit ff81838 into master Dec 5, 2025
6 checks passed
tpapp referenced this pull request Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inverse eltype calculation fails for vectors that do not contain scalars

4 participants