Conversation
TakKanekoGit
left a comment
There was a problem hiding this comment.
It's remarkable how compact and simple the code set for imu_dead_reckoning.py it's been refactored. It's really nice.
One issue (and it needs to be checked by Claude) is that it does not seem to account for the camera-to-scope alignment and it writes the camera-centre coordinates to solved["RA"]andsolved["Dec"].
Otherwise it looks great!
| pointing = imu_dead_reckoning.predict(q_x2imu) | ||
| assert pointing is not None # guaranteed by the is_initialized() check above | ||
| ra_deg, dec_deg, roll_deg = pointing.get(deg=True) | ||
| solved["RA"], solved["Dec"], solved["Roll"] = ra_deg, dec_deg, roll_deg |
There was a problem hiding this comment.
I think pointing estimated by imu_dead_reckoning is at the camera centre and it's no longer converted to the scope pointing. pointing is assigned here to both solved["camera_center"] and solved["RA"] etc.
There was a problem hiding this comment.
Indeed, it's just using the single solution in both places now. I'm hoping to remove the camera_center data entirely and just have the single set of pointing data flowing through. I still need to double check where that's used (probably align chart) and make sure to fix that up.
| # The IMU's unkonwn drifting reference frame X. This is solved for | ||
| # every time we have a simultaneous plate solve and IMU measurement. | ||
| q_eq2x: quaternion.quaternion = quaternion.quaternion(np.nan) # nan means not set | ||
| q_imu2pointing: quaternion.quaternion |
There was a problem hiding this comment.
I think pointing refers to the camera center but I think it's ambiguous because I first thought it meant the scope. May I propose putting it back to using the cam convention? So here q_imu2cam and so on?
There was a problem hiding this comment.
Hmmm... this is a good point. If we move the mapping between the camera pointing location and the telescope pointing location back into the solver using the pixel offset, then we are only carrying through the one RA/Dec/Roll out of the solver... but pointing is pretty ambiguous.
We could make it very explicit q_imu2platesolve?
There was a problem hiding this comment.
Sorry... I shouldn't try to do this in between my day job tasks 😅 You are totally right, the term in question is the physical orientation of the camera and IMU set by the screen direction. I'm renaming it back to q_imu2cam as it clearly confused me!
There was a problem hiding this comment.
Sounds good :-)
If it helps, I looked at tetra3 and it seems to use RA and Dec for camera/image centre and RA_target and Dec_target for the coordinates at the target_pixel.
| pointing = imu_dead_reckoning.predict(q_x2imu) | ||
| assert pointing is not None # guaranteed by the is_initialized() check above | ||
| ra_deg, dec_deg, roll_deg = pointing.get(deg=True) | ||
| solved["RA"], solved["Dec"], solved["Roll"] = ra_deg, dec_deg, roll_deg |
There was a problem hiding this comment.
Just a comment that after the integrator, we don't need to set the roll for scope: solved["Roll"]. It's there for historical reasons but it's no longer needed. No action needed. Just a note.
There was a problem hiding this comment.
Just to make sure I understand... we don't need this any longer as the orientation of the chart is no longer driven by it after your recent changes?
There was a problem hiding this comment.
Yes. That's correct. We just need to output RA/Dec and Az/Alt (but only when in Alt/Az mount mode) from the integrator.py
|
Thanks @TakKanekoGit for the review, it's very appreciated! For this refactor, I'm heading back to relying solely on using the pixel offset in Tetra3 to get the RA/DEC of the pixel selected during alignment and then feed this through as the RA/DEC/Roll that everything else uses. So it's sort of baked in early as part of the plate solving... but we do loose the two solutions (aligned and camera center), but it reduces the calculations needed and hopefully simplifies most of the code that now only needs to deal with a single pointing location. I think that the camera center only ended up only being used for the alignment UI. I need to do a followup or addition to this PR to fully remove that distinction and make sure the align screen still works as intended, probably by temporarily zeroing out the pixel offset while the alignment chart is being drawn 🤔 Another thing I'm keen to do, and may do it as part of this PR, is to replace all of the untyped dictionaries involved in the solving and integrator with dataclasses to make the whole thing much more legible and robust. Right not it's not at all clear what all the key's in the dictionary does, which are required where, and it's just a bit of mess that I've created 😅 |
The body rotation set from screen_direction is the IMU-to-camera hardware geometry; naming it for that physical relationship is clearer than naming it for its role in the math. Docstring notes that the class treats the camera frame as the pointing frame. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
camera_center duplicated the IMU-tracked camera FoV pointing that's already in top-level RA/Dec/Roll after the ImuDeadReckoning refactor. The chart in non-align mode now reads top-level RA/Dec/Roll directly; align mode continues to use camera_solve (frozen at the last plate solve). The integrator's solve() input switches to camera_solve, which is value-equivalent at solve time and the right semantic source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hi @brickbots
Although it adds an extra layer, I think we need to use the camera/image centre as the reference for seamless dead-reckoning with the IMU. This is because |
I have a wish list item that's completely irrelevant to this PR but it would be great if the PiFinder could remember the previous alignment and continue using it. For testing, I often have to restart the PiFinder so it'll be great if I don't have to re-align. As a user, I've had the battery die and had to re-align after plugging in a Power Bank 😀. |
That'll be really nice. You might have already spotted it but if you're able to use the In the same file ( Wouldn't it be better to do it in a different PR because that's quite a big change? |
This is an exploratory PR to see if several of the intermediate calculations can be dropped from the ImuDeadReckoning Class. Tests have been added to help validate the refactor and the previous implementation has been saved for side-by-side value validation.
The math involved here is not at all my strong point, so I relied on Claude for implementing my intuition that a few of these terms could drop out, especially if the existing Tetra3 camera pixel space offset alignment is relied on.
@TakKanekoGit this will really need your eyes to see if I've subtly broken something. The tests all look good and it seems to produce the same results. There may be further caching/optimization possible
Calculation Changes
Plate-solve update (solve)
the identity q_cam2imu · q_x2imu.conj() = (q_x2imu · q_imu2cam).conj()), but it removes the separately stored q_cam2imu = q_imu2cam.conj() — the conjugate is taken inline at the one site that needs
it.
Dead-reckoning (predict)
q_cam2scope). New: single three-term product q_eq2pointing = q_eq2x · q_x2imu · q_imu2pointing, with q_imu2pointing standing in for q_imu2cam.
Alignment calculation — removed entirely
Sentinel / NaN handling
Per-screen-direction q_imu2pointing table
Verified by the 14 cross-check tests in test_imu_dead_reckoning_equivalence.py: under identity alignment, the new predict() matches the old get_scope_radec() to 1e-9 radians across all six screen
directions through mixed solve/predict sequences.
Code Changes
Class changes (pointing_model/imu_dead_reckoning.py, ~230 → ~110 lines):
pre-composed once; downstream math is one quaternion-triple multiply per update instead of two.
predictions are computed on demand.
Integrator (integrator.py):
Tests:
1e-9 across all six screen_direction values.
Temporary scaffolding (delete in follow-up PR after field validation):
Test plan