Measurements.jl support, and event times at the observer's element type - #122
Merged
Conversation
The inner constructor defaulted alt and horiz to zero(T), which requires the element type alone to determine a zero. That holds for the types the package already supports, but not for every Real subtype: DynamicQuantities.RealQuantity refuses zero of a bare type because the dimensions live on the instance rather than in the type. Taking the defaults from lat instead is a no-op for Float32, Float64, Float128, BigFloat, ForwardDiff duals, and Measurements, and lifts the requirement for anything else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ9zmXUHQtA5suiMB8UdA7
The algorithms are generic over the element type, so Measurement observers work with no extension package. This adds a testset that pins down the behaviour rather than assuming it keeps working. The propagated uncertainty is checked against a finite-difference gradient for every algorithm, so the tests catch a wrong derivative chain and not merely a crash. A separate testset asserts that elevation plus zenith carries no uncertainty at all, which holds only while the observer's precomputed sin_lat and cos_lat stay correlated with latitude. Also covered: uncertain refraction parameters widening the apparent angles while leaving the geometric ones untouched, the vectorized, in-place and table paths, and the fact that the sunrise and sunset helpers drop the uncertainty because a DateTime cannot carry one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ9zmXUHQtA5suiMB8UdA7
Adds a worked refraction example that pairs an exact observer with an uncertain atmosphere, so the geometric angles come back exact while the apparent ones carry the pressure and temperature uncertainty. Also quantifies what treating elevation and zenith as independent would report, and notes that the element type, vectorized, in-place and table paths behave the same way. Trims the Observer constructor comment and drops the reference to a specific package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ9zmXUHQtA5suiMB8UdA7
Contributor
Benchmark Results (Julia vlts)Time benchmarks
Memory benchmarks
|
Contributor
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Contributor
Benchmark Results (Julia vpre)Time benchmarks
Memory benchmarks
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 754 764 +10
=========================================
+ Hits 754 764 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
transit_sunrise_sunset rounds the event times to a whole second in order to build a DateTime. That discards the sub-second part, and with it anything the element type carries beyond a value. The solve already runs at the observer's element type, so the information exists right up to that final conversion and is then thrown away. transit_sunrise_sunset_seconds returns the same events as seconds since midnight UTC, keeping the element type. A Measurement observer therefore yields event times with an uncertainty, and a dual-valued observer yields differentiable ones. Neither was possible before, and the DateTime variant could not even be finite-differenced, since a step small enough to be a derivative is swamped by the rounding. The conversion now dispatches on the requested type, so the DateTime and ZonedDateTime paths are untouched, including the polar day and night branch. TransitSunriseSunset needed no change, having been made generic when TimeZones moved to an extension. Covered from both directions: the propagated uncertainty is checked against a finite-difference gradient, the ForwardDiff derivative against the same, and the two variants are asserted to describe the same instant for a Float64 observer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ9zmXUHQtA5suiMB8UdA7
langestefan
changed the title
Codecov flagged two uncovered lines in srt.jl. The cause was that nothing called transit_sunrise_sunset_seconds with a Date, so that method was never compiled. Adds a case for it, asserting it agrees with passing midnight of the same day as a DateTime, and one for the polar branch, which has to return midnight at the requested element type rather than falling back to a DateTime. The polar line was already covered by the DateTime path, so it was not the coverage failure, but it is behaviour this change introduced and was going untested. A full run with coverage now reports no uncovered lines anywhere in src. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ9zmXUHQtA5suiMB8UdA7
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
1 participant
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.
Measurements.jl already works with the package, since every algorithm is generic over the
element type. Nothing enforced that, so this adds a testset that pins it down and a docs
section. Testing it then turned up one real gap and one small constructor wart, both fixed
here.
Tests
test/positioning/test-measurements.jl, written to fail on a wrong answer rather than onlyon a crash:
algorithms, on both elevation and azimuth, so a broken derivative chain is caught
elevation + zenithmust be exactly90.0 ± 0.0. This holds only while the observer'sprecomputed
sin_latandcos_latstay correlated with latitude, which is the part mostlikely to go silently wrong. Uncorrelated propagation would report
± 0.014ones untouched
Measurement{Float32}stayingFloat32Event times at the observer's element type
transit_sunrise_sunsetrounds to a whole second to build aDateTime, discarding thesub-second part and anything the element type carries beyond a value. The solve already runs
at the observer's element type, so that information exists right up to the final conversion
and is then dropped. With a
Measurementobserver, roughly 4 s of uncertainty on sunrise wasbeing computed and discarded.
transit_sunrise_sunset_secondsreturns the same events as seconds since midnight UTC,keeping the element type:
This is not only for Measurements. It also makes the events differentiable, which they were
not before, and not workaroundable either: the
DateTimevariant cannot be usefullyfinite-differenced, because a step small enough to behave like a derivative is swamped by the
whole-second rounding. As a cross-check, ForwardDiff gives
-354.9 s/degfor latitude and-240.0 s/degfor longitude, the latter being exactly 4 minutes per degree. Recombining thosegives
4.28 s, matching what Measurements reports by an entirely separate route.The conversion dispatches on the requested type, so the
DateTimeandZonedDateTimepathsare untouched, including the polar day and night branch.
TransitSunriseSunsetneeded nochange, having been made generic when TimeZones moved to an extension.
Observer constructor
The inner constructor defaulted
altandhoriztozero(T), requiring the element typealone to determine a zero. Taking them from
latinstead is a no-op forFloat32,Float64,Float128,BigFloat, ForwardDiff duals, and Measurements, and lifts the requirement forRealsubtypes that carry information on the instance.Verification
Pkg.test()passes: Measurements 88, Autodiff 117, Aqua 11/11, JET 1/1, and everypre-existing testset unchanged, including Srt at 517. The three
Brokenin Sg2 arepre-existing
@test_skipsprek run --all-filespasses, including Runic and ExplicitImportsNot covered
The constructor change has no direct test. Exercising it needs a
Realsubtype withoutzero(::Type), and adding a contrived one seemed worse than leaving it, given every supportedtype behaves identically either way.
A full
docs/make.jlbuild was not run locally; the example snippets were verified on theirown and CI covers the rest.
🤖 Generated with Claude Code