Skip to content

fix: Fix PHTCNT, ISPC, and RN header datatypes#747

Open
michaelliangau wants to merge 37 commits intoroman-corgi:developfrom
michaelliangau:fix-header-datatypes
Open

fix: Fix PHTCNT, ISPC, and RN header datatypes#747
michaelliangau wants to merge 37 commits intoroman-corgi:developfrom
michaelliangau:fix-header-datatypes

Conversation

@michaelliangau
Copy link

@michaelliangau michaelliangau commented Feb 13, 2026

Describe your changes

Fixing incorrectly specified headers (PHTCNT, ISPC and RN) throughout the pipeline and documentation.

  • PHTCNT: boolstr ("True", "False", or "Auto")
  • ISPC: boolint (0 or 1)
  • RN: str/intfloat

Default value for PHTCNT is now "Auto".

Additional context

E2E Test Results

All 33 e2e tests passed with v3.3 test data. 28/29 zz_dataformat checks passed — the one failure (bpmap CTI_CORR/DESMEAR type mismatch) is a pre-existing issue on develop, not introduced by this PR.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Reference any relevant issues (don't forget the #)

#632

Checklist before requesting a review

  • I have verified that all unit tests pass in a clean environment and added new unit tests, as appropriate
  • I have checked that I am merging into the right branch
  • I have checked the output of the latest Github Actions run associated with this PR and confirmed running pytest did not produce any warnings
  • I have checked if my code modifies an existing step function or modifies the data format docs. If it does, I've added my PR to the list maintained here.

michaelliangau and others added 30 commits January 21, 2026 17:37
Update ISPC field across documentation and code to use integer convention
(0 or 1) instead of boolean (False or True). ISPC=1 indicates photon
counting mode enabled, ISPC=0 indicates analog mode.

Changes:
- Updated 18 data format documentation files (.rst)
- Updated walker.py to check ISPC == 1 instead of ISPC in (True, 1)
- Updated test files to use 0/1 instead of False/True
- Updated error messages and comments to reflect 0/1 convention

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Change ISPC definition in l1.csv from bool/FALSE to int/0 to match
the integer convention used throughout the codebase.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Change ISPC from bool (FALSE) to int (0) to correctly represent
photon counting mode as 0 or 1 instead of boolean values.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolved conflicts by accepting develop's versions for docs/data_formats
and e2e test files. Develop already has the PHTCNT/ISPC int datatype
fixes in docs, and the test helper functions have been refactored.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@michaelliangau
Copy link
Author

michaelliangau commented Feb 13, 2026

Feedback from Julia

  • PHTCNT may need to be "Auto". Change int --> str? Confirm with Alexandra Greenbaum
  • Don't change e2e test data, run tests with v3.3 and rewrite headers in e2e etsts if necessary

@mygouf mygouf assigned semaphoreP and unassigned semaphoreP Feb 13, 2026
@mygouf mygouf requested a review from semaphoreP February 13, 2026 22:34
@michaelliangau michaelliangau marked this pull request as ready for review February 14, 2026 18:36
@semaphoreP
Copy link
Contributor

@michaelliangau can you push your branch to the corgidrp repo (instead of using your forked repo?). It's very clunky for me to run e2e tests for verification from your forked repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants