Repository navigation
Conversation
parse_data took the 18-bit phase and 17-bit baseline fields but sign-extended them from bit 17 and bit 16. Every phase beyond -65536 wrapped to a large positive value: on MEC 850 nm laser data this is 10% of photons, the largest pulses, which downstream negative-phase selection then discards. Sign-extend from the full field widths. DataPacket::serialize started from the 0xFF header marker, so written data packets read back as headers. Data packets now carry no marker. Adds a round-trip test with full-width values, including a real MEC photon (phase -100212), and pins the encoding of the existing test packet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCeGEREfv2ZiuVXVvm7nF1
ld-cd
requested changes
Oct 11, 2026
ld-cd
left a comment
ld-cd
left a comment
Contributor
There was a problem hiding this comment.
A human written PR message thats a bit more concise would be appreciated in the future
|
|
||
| assert_eq!(input_data, stream(&[])) | ||
| assert_eq!(input_data, stream(&[])); | ||
| let packet = DataPacket { x: 99, y: 44, timestamp: 131, phase: -34148, baseline: -4334 }; |
Contributor
There was a problem hiding this comment.
This should be a seperate test as serialization and deserialization are very nearly orthogonal as implemented here
| } | ||
|
|
||
| #[test] | ||
| fn test_data_full_width_sign_extension() { |
Contributor
There was a problem hiding this comment.
something a little less verbose like test_sign_extension
This branch has not been deployed
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.
parse_datareads the 18-bit phase and 17-bit baseline fields but sign-extends them from bit 17 and bit 16. Every phase below -65536 therefore wraps to a large positive value, and baselines beyond ±32768 wrap the same way.The effect on real data is large. In the MEC 850 nm laser file
rawdata/20201006/1602040830.bin, 10% of photons have a phase below -65536. These are the largest pulses: the bluest photons on the most responsive pixels. bolide keeps only negative phases, so it silently dropped all of them, which biased both the wavelength calibration and the science spectra. In stores regenerated with this fix, 8% of MEC photons and 18% of XKID photons fall in that range.Changes
DataPacket::serializeno longer starts from the 0xFF header marker. Data packets written byserialize_dataframeused to read back as headers.Verification
cargo test --lib: 5 passed.1602040830.binwere decoded independently in numpy, using the 18/17-bit packet layout of mkidcore'sbinprocessor.c, and joined to binney's output on (xy, timestamp). Phase and baseline agree for 100% of photons, and no positive phase is stored. Before the fix, binney's phase equaled the 17-bit wrap of the true value for every photon.Parquet written by earlier versions should be reconverted.
A related issue is left for a separate change. Output files are created before they are written, so a crash leaves zero-byte
.parquetfiles that later runs skip as already converted. One interrupted conversion here left 870 of them. Writing to a temporary name and renaming on completion would prevent this.🤖 Generated with Claude Code
https://claude.ai/code/session_01QCeGEREfv2ZiuVXVvm7nF1