Skip to content

Add SmallTag type for more compact Dual types #748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

topolarity
Copy link

This is largely to improve printing and reduce visual noise for packages with large function types (e.g. SciML)

Compare tag = :small:

ForwardDiff.Dual{ForwardDiff.SmallTag{0x39f35d61d979c3d1}, Float64, 3}

to tag = :default:

ForwardDiff.Dual{ForwardDiff.Tag{var"#f#1"{DAEProblem{Vector{Float64}, Vector{Float64}, Tuple{Float64, Float64}, false, Vector{Float64}, DAEFunction{false, SciMLBase.FullSpecialize, typeof(f), Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, typeof(SciMLBase.DEFAULT_OBSERVED), Nothing, Nothing, Nothing}, @Kwargs{}, Vector{Bool}}, BrownFullBasicInit{Float64, Nothing}, DFBDF{5, 0, ADTypes.AutoForwardDiff{nothing, Nothing}, Nothing, OrdinaryDiffEqNonlinearSolve.NLNewton{Rational{Int64}, Rational{Int64}, Rational{Int64}, Rational{Int64}}, typeof(OrdinaryDiffEqCore.DEFAULT_PRECS), Val{:forward}(), true, nothing, Nothing, Nothing}}, Float64}, Float64, 3}

@topolarity topolarity changed the title Add SmallTag type for more compute Dual types Add SmallTag type for more compact Dual types Apr 23, 2025
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 86.36364% with 9 lines in your changes missing coverage. Please review.

Project coverage is 86.92%. Comparing base (c310fb5) to head (d88bf58).
Report is 21 commits behind head on master.

Files with missing lines Patch % Lines
src/config.jl 86.36% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #748      +/-   ##
==========================================
- Coverage   89.57%   86.92%   -2.66%     
==========================================
  Files          11       10       -1     
  Lines         969     1025      +56     
==========================================
+ Hits          868      891      +23     
- Misses        101      134      +33     

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

@mcabbott
Copy link
Member

We could also revive #572, closed it was thought that SciML printing problems were solved in Base.

Can you comment on the comparison?

@topolarity
Copy link
Author

topolarity commented Apr 23, 2025

We could also revive #572, closed it was thought that SciML printing problems were solved in Base.

Base has type-folding now that helps many stack traces, but you still sometimes need a repr(...) of a type that you can copy-and-paste, which disables folding

Can you comment on the comparison?

The main difference is that debugging perturbation issues without the actual function / array types can be very confusing, so it seems useful to keep the original functionality around. Other than that, the implementation should be mostly equivalent (the hash is based in many cases on the objectid)

Also it's worth mentioning that tag = short does not allow the Dual / Config type to be inferred on 1.10 sadly, since hash(::Type) is only const-eval-able on 1.11+ (see JuliaLang/julia#52427 (comment))

This is an alternative to `Tag` that provides largely the same
functionality, but carries around only the hash of the function
/ array types instead of the full types themselves.

This can make these types much less bulky to print and easier to
visually scan for.
This provides a convenient interface to ask for a SmallTag.
Latest patch release seems to have tightened this up.
This old version of Julia doesn't have call-site `@inline` / `@noinline`
so version-guard against this trick for now.
@ChrisRackauckas
Copy link
Member

Chatted on this with @oscardssmith and @topolarity . This is better than using the objectid because it is consistent with respect to precompilation, i.e. in a new session the objectid can change depending on the order that you evaluate functions, while this is consistent. That plus on v1.11 this is const eval-able, and thus using this form of a short tag is type-stable and makes it so you will precompile the autodiff calls if you autodiff w.r.t. the same function.

With those two properties though, I would be highly in favor of defaulting to this on v1.11+, since it won't hurt performance or precompilation, and I cannot see a scenario where a normal user would favor the tags based on function types. It would be odd to have huge multi-screen types on v1.10 and then good stack traces on v1.11, but IMO that's not a reason to make it wait longer.

I'd like to hear @KristofferC's opinion here. But either way, I think we'd want to set this up as well in ADTypes and DifferentiationInterface @gdalle

Copy link
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

I like the general idea! I think #724 and #744 should be finished and merged first though

end

@generated function tagcount(::Type{SmallTag{H}}) where {H}
:($(Threads.atomic_add!(TAGCOUNT, UInt(1))))
Copy link
Member

Choose a reason for hiding this comment

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

This seems related to #724, maybe someone can review that PR first?

Comment on lines +103 to +106
if kind === :default
return Tag(f, X)
elseif kind === :small
return SmallTag(f, X)
Copy link
Member

Choose a reason for hiding this comment

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

This is type-unstable, can we do something about it? Perhaps pass the symbol as a Val?

Copy link
Member

Choose a reason for hiding this comment

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

One of my objections to this PR is that I don't much like adding keyword tag::Union{Symbol,Nothing} = :default to everything. (1) it's messy, and I'm not sure how often anyone needs to pass this, (2) accepting a symbol from a special list of 2 seems like an odd interface. I'm not sure Val(:default) is much better. (3) it's called tag but isn't the tag, doesn't accept aTag.

Maybe a better interface would be to use Preferences.jl, as for NaN-safe mode?

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines +27 to +31
# SmallTag is similar to a Tag, but carries just a small UInt64 hash, instead
# of the full type, which makes stacktraces / types easier to read while still
# providing good resilience to perturbation confusion.
struct SmallTag{H}
end
Copy link
Member

Choose a reason for hiding this comment

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

This should be an actual docstring

Copy link
Member

Choose a reason for hiding this comment

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

And it should be marked public or exported, but so should other symbols (#744)

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this needs a new type at all? Could we just extend Dual's first type parameter from Union{Nothing, Tag} to perhaps Union{Nothing, Tag, Integer}?

julia> ForwardDiff.Dual(pi/2, 1)
Dual{Nothing}(1.5707963267948966,1.0)

julia> ForwardDiff.derivative(x -> @show(x), pi/2)
x = Dual{ForwardDiff.Tag{var"#37#38", Float64}}(1.5707963267948966,1.0)
1.0

Copy link
Member

Choose a reason for hiding this comment

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

If it does need a new type, the obvious name is HashTag.

@topolarity
Copy link
Author

I think #724 and #744 should be finished and merged first though

I'm not really convinced that #724 is a proper fix for the issue it claims to solve - it is still possible for the counter to overlap, e.g., between multiple precompiled packages.

I am happy to update the type exports while we're at it though

@gdalle
Copy link
Member

gdalle commented May 12, 2025

I'm not really convinced that #724 is a proper fix for the issue it claims to solve - it is still possible for the counter to overlap, e.g., between multiple precompiled packages.

I don't feel confident enough to evaluate that one, but I thought it looked related to what you did with SmallTag

Copy link
Member

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

Not a full review but some comments:

  • If we want to keep two methods of tagging around, is this the right way to access them? It seems quite an obscure internal detail, which should never change any results. So perhaps need not be a new keyword on every user-facing function.

  • I wonder if the implementation can be much simpler, like just storing the integer as the tag, instead of adding more structs.

Comment on lines +103 to +106
if kind === :default
return Tag(f, X)
elseif kind === :small
return SmallTag(f, X)
Copy link
Member

Choose a reason for hiding this comment

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

One of my objections to this PR is that I don't much like adding keyword tag::Union{Symbol,Nothing} = :default to everything. (1) it's messy, and I'm not sure how often anyone needs to pass this, (2) accepting a symbol from a special list of 2 seems like an odd interface. I'm not sure Val(:default) is much better. (3) it's called tag but isn't the tag, doesn't accept aTag.

Maybe a better interface would be to use Preferences.jl, as for NaN-safe mode?

Comment on lines +27 to +31
# SmallTag is similar to a Tag, but carries just a small UInt64 hash, instead
# of the full type, which makes stacktraces / types easier to read while still
# providing good resilience to perturbation confusion.
struct SmallTag{H}
end
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this needs a new type at all? Could we just extend Dual's first type parameter from Union{Nothing, Tag} to perhaps Union{Nothing, Tag, Integer}?

julia> ForwardDiff.Dual(pi/2, 1)
Dual{Nothing}(1.5707963267948966,1.0)

julia> ForwardDiff.derivative(x -> @show(x), pi/2)
x = Dual{ForwardDiff.Tag{var"#37#38", Float64}}(1.5707963267948966,1.0)
1.0

Comment on lines +59 to +60
@inline function ≺(::Type{SmallTag{H1}}, ::Type{SmallTag{H2}}) where {H1,H2}
tagcount(SmallTag{H1}) < tagcount(SmallTag{H2})
Copy link
Member

@mcabbott mcabbott May 12, 2025

Choose a reason for hiding this comment

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

This might be obvious, but can you explain a bit why this is designed this way? Why calculate hash H and store that in the type, but call the generated tagcount(SmallTag{H1}) to get a different integer whenever you use it. Why not store the integer from TAGCOUNT as a type parameter directly?

@gdalle
Copy link
Member

gdalle commented May 13, 2025

Also, am I correct that this doesn't make perturbation confusion impossible, only vanishingly unlikely?

@oscardssmith
Copy link
Member

@gdalle that is correct. With a 64 bit hash, we have a 50% collision chance once you get to 4 billion different tags (that are used in the same operation), but as long as your number of tags is below millions (which should always be the case), the chance of confusion is incredibly unlikely

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.

5 participants