Fix laser equality/hashing and actually run the thread-safety test#55
Merged
Conversation
- Include test/threads.jl in the test suite; the CI threads matrix never actually exercised it. - Exclude the task-local scratch cache from ==, isequal and hash: it only holds transient intermediate results, so including it made equality and hashes depend on the evaluation history of the current task (and hashing allocated a cache as a side effect). This also fixes a copy-paste bug in isequal which compared geometry(a) against mutable_cache(b), and makes the laser parameters (e.g. phi0) part of equality. - Bump the minimum Julia version to 1.10; the declared julia = "1" compat was already unsatisfiable since TaskLocalValues requires a newer Julia. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Hx2cg1rQe5TmeWiy31HCv
On the 1.13 pre-release JET reports runtime dispatch from upstream code (Base printing/broadcast internals, StaticArrays construction) as well as inference regressions surfacing at LaserTypes call sites, so restrict the QA testset to release versions of Julia. Also fix the CI job name template (matrix.julia-version -> matrix.version) so the Julia version shows up in job names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Hx2cg1rQe5TmeWiy31HCv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up fixes from a review of the multi-threading implementation (the
TaskLocalValue-based scratch caches introduced in #54 are sound; these are the issues found around them).Changes
Run the thread-safety test in CI
test/threads.jlexisted and the CI matrix runs withJULIA_NUM_THREADS: 1/2specifically for it, buttest/runtests.jlnever included it — so the regression fixed in #54 had no test coverage. It is now part of the test suite (@safetestset "Threads").Exclude the task-local scratch cache from
==,isequalandhashThe cache only holds transient intermediate results of the last field evaluation, so including it made equality and hashing depend on the evaluation history of the current task: a laser's hash changed after every
E/Bcall (breaking lasers asDict/Setkeys), two identical lasers compared unequal if only one had been evaluated, and merely comparing or hashing a laser allocated a cache for the calling task.The new implementations compare/hash all fields except
cache. This also:isequal, which comparedgeometry(a)againstmutable_cache(b)and therefore always returnedfalse;λ,a₀,ϕ₀,w₀, …) part of equality — previously two lasers differing only inϕ₀compared equal.Bump minimum Julia version to 1.10
The declared
julia = "1"compat was already unsatisfiable in practice since TaskLocalValues requires a newer Julia. The package version inProject.tomlis left untouched — bump it as you see fit when releasing.Notes
setup_laserequality tests intest/setup.jlstill pass with the stricter==.laser.cache[]lookup so each field evaluation does one task-local lookup instead of ~11, and the longer-term refactor to pass an immutable intermediates struct instead of mutating a cache.🤖 Generated with Claude Code
https://claude.ai/code/session_012Hx2cg1rQe5TmeWiy31HCv
Generated by Claude Code