Prevent infinite recursion of unit by adding a method unit(::Type{Any}) #807

SeSodesa · 2025-08-29T09:22:08Z

This is to resolve errors with including units into FiniteDifferences.jl. See JuliaDiff/FiniteDifferences.jl#244 for details.

Tests for Units.jl need to be updated (the @test_throws ones), if this is to be accepted. The totalness of unit introduced via unit(::Any) might be a bit controversial, but I don't think that should be a problem. In general, things don't have units. Only (some) Numbers do.

Closes #806.

…it(::Type{Union{Missing,T}}) This is to resolve errors with uncluding units into FiniteDifferences.jl. See JuliaDiff/FiniteDifferences.jl#244 for details. Tests for Units.jl need to be updated, if this is to be accepted. The totalness of unit introduced via unit(::Ay) might be a bit controversial, but I don't think that should be a problem. In general, things don't have units. Only Numbers do. Closes JuliaPhysics#806.

SeSodesa · 2025-08-29T13:33:41Z

Looks like there are not too many tests failing because if this change, but there are apparently things like dates that should be throwing because of some reason that I still need to decipher. Any comments on this would be appreciated.

SeSodesa · 2025-08-29T16:59:38Z

Ah, right, the methods were intentionally missing for date-type units where a duration cannot be fixed sensibly. I'll need to modify this a bit to keep those semantics intact.

…e types with ambiguous units For example, Years are not always the same length, so they do not have unambiguous units. Many abstract Date types also do not have units.

codecov · 2025-08-30T10:25:26Z

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@c1a9b4e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/utils.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #807   +/-   ##
=========================================
  Coverage          ?   89.46%           
=========================================
  Files             ?       21           
  Lines             ?     1699           
  Branches          ?        0           
=========================================
  Hits              ?     1520           
  Misses            ?      179           
  Partials          ?        0

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sostock

Restricting T in the method unit(::Type{Union{Missing, T}}) is the correct thing to do to fix the stack overflow. Adding unit(::Any) and unit(::Function) is not.

src/utils.jl

@sostock

As suggested by @sostock, do not only restrict the type of T to Number, but include FixedPeriod as well, to accommodate Dates-soecific code. Co-authored-by: Sebastian Stock <[email protected]>

…:Type{Union{Missing, T}}) and remove unwanted unit methods Now the tests run again. Still need to revert the tests that were converted from throwing MethodError to outputting NoUnits.

…ng against NoUnits This was to accommodate the removed unwanted methods unit(::Any) and unit(::Function).

SeSodesa · 2025-09-01T11:49:39Z

I changed the title of the pull request to reflect its scope. I will look into why FiniteDifferences.jl does what it does more deeply. At the very least I am currently trying to take the unit of a function being differentiated in JuliaDiff/FiniteDifferences.jl#244. A quick look indicates that estimate_step might allow both containers and functions as input, so allowing unit(::Any) with unit(eltype(f)) made it seem like it functioned as intended.

SeSodesa · 2025-09-01T11:53:04Z

And eltype of T<:Function is any.

sostock · 2025-09-01T18:57:45Z

After thinking some more about this, I’m not sure this is necessary.

The stack overflow only happens when someone calls unit(Any). With this PR, unit(Any) throws a MethodError instead of a StackOverflowError. So it only changes what kind of error is thrown.

Before this PR, a user could define unit(::Type{MyType}) any would get a working unit(::Type{Missing, MyType}) via this method. After this PR, this is only the case if MyType <: Union{Number, Dates.FixedPeriod}. Therefore, this could actually be considered breaking. However, I don’t think it is likely that someone defines unit on a type that is not <: Number, so this may not matter.

SeSodesa · 2025-09-01T20:11:14Z

The different kind of error might be a good thing, though. It could function as an explicit way of telling a user that they misimplemented something, which is what I did and spent some time tracking down what and why, especially since the purpose of each method is not documented.

The method unit(::Any) could throw a specific error with a more informative error message, such as

function unit(x::Any)

    NotImplementedError("You tried calling unit on an object $x, that has none defined. Not everything has (or should have) units.")

end

User experience, and all that. StackOverflowErrors are never very nice.

SeSodesa · 2025-09-01T20:13:07Z

Then the type restriction could be lifted to allow defining unit on type other than T <: Number.

This allows us to remove the type restriction on T for unit(::Type{Union{Missing,T}}), making it possible for users to define unit on their own types, while at the same time giving a more descriptive error message than StackOverflowError.

src/utils.jl

…e{Union{Missing,T}}) Not needed after re-removal of type constraint. Co-authored-by: Sebastian Stock <[email protected]>

…urposes Also changed the exception type thrown by unit(::Type{Any}) to ArgumentError, and changed the error description to be more helpful.

…moval of unit(::Any)

SeSodesa · 2025-09-03T18:40:07Z

Had to force push to amend a commit message. But maybe this is close to completion now.

src/utils.jl

Co-authored-by: Sebastian Stock <[email protected]>

The docstring is not needed, as this is not a feature that users need to know about in the documentation.

src/utils.jl

…ssage Line break escaping not supported in multi-line strings in older Julia versions. Co-authored-by: Sebastian Stock <[email protected]>

sostock

Just two small comments to make the code style consistent with the rest of the package. Then this is ready to merge.

src/utils.jl

No empty lines between comments and code, and no spaces after opening parentheses of function invocations. Co-authored-by: Sebastian Stock <[email protected]>

Remove space between closing parentheses of nested function invocations Co-authored-by: Sebastian Stock <[email protected]>

sostock · 2025-09-15T08:37:13Z

Thanks!

SeSodesa marked this pull request as draft August 29, 2025 09:22

SeSodesa changed the title ~~Define unit(::Any)m unit(::Funciton) and add a type restriction to unit(::Type{Union{Missing,T}})~~ Define unit(::Any), unit(::Function) and add a type restriction to unit(::Type{Union{Missing,T}}) Aug 29, 2025

test/dates: assume NoUnits instead of MethodErrors in the case of Dat…

51e81a5

…e types with ambiguous units For example, Years are not always the same length, so they do not have unambiguous units. Many abstract Date types also do not have units.

SeSodesa marked this pull request as ready for review August 30, 2025 11:56

sostock requested changes Aug 31, 2025

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

SeSodesa and others added 2 commits September 1, 2025 13:48

Loosen the type restrictions of unit(::Type{Missing,T})

4f2bf7f

As suggested by @sostock, do not only restrict the type of T to Number, but include FixedPeriod as well, to accommodate Dates-soecific code. Co-authored-by: Sebastian Stock <[email protected]>

Fully qualify Dates.FixedPeriod in type restriction of method unit(x:…

c230a6a

…:Type{Union{Missing, T}}) and remove unwanted unit methods Now the tests run again. Still need to revert the tests that were converted from throwing MethodError to outputting NoUnits.

SeSodesa force-pushed the make-unit-function-total branch from 8712763 to c230a6a Compare September 1, 2025 11:02

Revert the Dates-related tests to throw MethodErrors instead of testi…

fe1e225

…ng against NoUnits This was to accommodate the removed unwanted methods unit(::Any) and unit(::Function).

SeSodesa changed the title ~~Define unit(::Any), unit(::Function) and add a type restriction to unit(::Type{Union{Missing,T}})~~ Prevent infinite recursion of unit by adding a type restriction to unit(::Type{Union{Missing,T}}) Sep 1, 2025

SeSodesa mentioned this pull request Sep 1, 2025

Add Unitful support JuliaDiff/FiniteDifferences.jl#244

Open

SeSodesa requested a review from sostock September 1, 2025 13:26

sostock requested changes Sep 3, 2025

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

SeSodesa and others added 3 commits September 3, 2025 21:19

Remove now-unnecessary comment about type restriction from unit(::Typ…

b43536b

…e{Union{Missing,T}}) Not needed after re-removal of type constraint. Co-authored-by: Sebastian Stock <[email protected]>

Remove method unit(::Any), and move unit(::Type{Any}) for docstring p…

e071b61

…urposes Also changed the exception type thrown by unit(::Type{Any}) to ArgumentError, and changed the error description to be more helpful.

Change Dates-related tests to expect MethodErrors again, after the re…

108bc4e

…moval of unit(::Any)

SeSodesa force-pushed the make-unit-function-total branch from 3d2e362 to 108bc4e Compare September 3, 2025 18:39

SeSodesa changed the title ~~Prevent infinite recursion of unit by adding a type restriction to unit(::Type{Union{Missing,T}})~~ Prevent infinite recursion of unit by adding a method unit(::Type{Any}) Sep 3, 2025

sostock requested changes Sep 4, 2025

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

SeSodesa and others added 2 commits September 5, 2025 15:57

Shorten error message thrown by unit(Any)

16c7404

Co-authored-by: Sebastian Stock <[email protected]>

Replace docstring of unit(::Type{Any}) with a simple comment

6320332

The docstring is not needed, as this is not a feature that users need to know about in the documentation.

sostock reviewed Sep 7, 2025

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

Take Julia 1.6 compatibility into account with the unit(Any) error me…

6af1980

…ssage Line break escaping not supported in multi-line strings in older Julia versions. Co-authored-by: Sebastian Stock <[email protected]>

sostock requested changes Sep 14, 2025

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

SeSodesa and others added 2 commits September 15, 2025 11:05

Follow code style of project

7ea4b1f

No empty lines between comments and code, and no spaces after opening parentheses of function invocations. Co-authored-by: Sebastian Stock <[email protected]>

Further code style changes

48b9629

Remove space between closing parentheses of nested function invocations Co-authored-by: Sebastian Stock <[email protected]>

sostock approved these changes Sep 15, 2025

View reviewed changes

sostock merged commit d14ad5b into JuliaPhysics:master Sep 15, 2025
15 checks passed

Prevent infinite recursion of unit by adding a method unit(::Type{Any}) #807

Prevent infinite recursion of unit by adding a method unit(::Type{Any}) #807

Uh oh!

Conversation

SeSodesa commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeSodesa commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeSodesa commented Aug 29, 2025

Uh oh!

codecov bot commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sostock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SeSodesa commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeSodesa commented Sep 1, 2025

Uh oh!

sostock commented Sep 1, 2025

Uh oh!

SeSodesa commented Sep 1, 2025

Uh oh!

SeSodesa commented Sep 1, 2025

Uh oh!

Uh oh!

Uh oh!

SeSodesa commented Sep 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sostock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sostock commented Sep 15, 2025

Uh oh!

Uh oh!

SeSodesa commented Aug 29, 2025 •

edited

Loading

SeSodesa commented Aug 29, 2025 •

edited

Loading

codecov bot commented Aug 30, 2025 •

edited

Loading

sostock left a comment •

edited

Loading

SeSodesa commented Sep 1, 2025 •

edited

Loading