Conversation
icweaver
marked this pull request as ready for review
July 19, 2026 04:15
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fk4 #106 +/- ##
==========================================
+ Coverage 95.16% 95.42% +0.26%
==========================================
Files 8 9 +1
Lines 310 328 +18
==========================================
+ Hits 295 313 +18
Misses 15 15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Example application for #43 (comment)
Depends on #101
Plan to merge into #43 / update the PR description with the below if things look alright to y'all (modulo self-references to #43)
Adds
ProjectedCoords: a flat-sky, small-field-of-view representation of a coordinate as an offset from an origin coordinate on top of the #105/#101 conversion design.ProjectedCoords{TC, T}stores anorigin::TCcoordinate and a 2-vectoroffset(longitude difference scaled bycos(lat(origin)), latitude difference, both in radians).project(origin, c)constructs one, convertingcintoorigin's frame first.Design: no
convertmethods at allA projected coordinate is a representation around an origin value, not a coordinate frame of its own, its frame is the origin's. #43 integrated it with two custom
Base.convertmethods, which are cross-ambiguous with theCartesianCoordstarget methods (more specific in the source, less specific in the target). After #105/#101, the right place to plug in is the primitives, and the whole integration is:frame_transformdelegation to the origin's frame (plus a one-line FK4 disambiguator, the same pairwise cost the EclipticCoords/FK4NoETermsrotmatpair pays). Every conversion out of aProjectedCoordsthen works generically: spherical targets,CartesianCoordstargets in every parameter spelling, KDTree matching, non-rotational origin frames (projecting around anFK4Coordsorigin works, E-terms included), and even nested projections, which chain through their origins._checkframeoverride makes every conversion into a projected type (which would need an origin value that no type can carry) throw one clearArgumentErrorpointing atproject(origin, c), instead of feat: add simple projected coordinates #43's recursion. Identity conversion (convert(typeof(cp), cp) === cp) still works.cartesian(proj)tags with the origin's frame.constructorofmust keep returningProjectedCoordsforsetpropertiesreconstruction, so this is the first type where reconstruction identity and frame identity diverge, possibly an argument for eventually giving the frame tag its own function rather than reusingconstructorof.==/hashon (origin, offset). The generic frame-tag equality from refactor!: Consistent conversion rules and a frame-tag design forCartesianCoords#105 sees only the bareProjectedCoordstag (the origin's frame is erased) so raw lon/lat numbers around origins in different frames would have compared equal. Comparing the origin (frame included) and the offset fixes that, with a matchinghash.Periodic-longitude
isapprox(first commit, extracted from #43)Constructors normalize longitude with
mod2pi, so two points straddling the lon = 0 wrap are stored ~2π apart, and the elementwise(lon, lat)comparison called them not-≈:ICRSCoords(-eps(), 1) ≈ ICRSCoords(0, 1)wasfalse. This is not just a constructed case, conversion round-trips genuinely land on the far side of the wrap (e.g.ICRSCoords(0.0, -1.372)comes back from a Galactic round-trip withlon = 6.283185307179586), soconvert(ICRSCoords, convert(GalCoords, c)) ≈ cfailed spuriously for sources at ra ≈ 0.The longitude difference is now taken in
(-π, π]viarem2pibefore comparing. Where #43 applied this to 3 of the 5 frames (leavingSuperGalCoordsandEclipticCoordswith the old behavior), it now goes through one_isapprox_lonlathelper used uniformly by all seven frames. Away from the wrap the comparison is unchanged.Breaking changes
isapproxnear thelon= 0 wrap now returnstruewhere it previously returnedfalse(that was the bug). Results away from the wrap are identical.CartesianCoords#105/feat: AddFK4CoordsandFK4NoETerms#101.