Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix gantry tilt float bug (#782) #791

Closed

Conversation

cfhammill
Copy link

Small change to fix #782. Testing clean in dcm_qa/master except for the presence of a new BIDS tag "BidsGuess" in the json files. Things were looking good in dcm_qa_uih but then I updated submodule's master branch and now I'm getting a handful of binary files differ messages. I didn't see anything obviously different in the header and the voxel data appears to be the same.
Results appear the same between this patch and the previous release version on dcm_qa_ct except for "BidsGuess" and the issues reported in neurolabusc/dcm_qa_ct#1

@neurolabusc
Copy link
Collaborator

@cfhammill this is compiler dependent. I can elicit the bug with

dcm2niiX version v1.0.20230411 Clang12.0.0 ARM (64-bit MacOS)

but not when the same code is compiled with a more modern compiler.

dcm2niiX version v1.0.20230411  Clang14.0.3 ARM (64-bit MacOS)

Can you please test a different modification than the one you suggested. Please change

if (!isSameFloat(cosX, 1.0))

to read

if (!isSameFloatGE(cosX, 1.0))

The former uses the machine dependent tolerance FLT_EPSILON while the latter uses the more liberal 0.0001. It is unfortunate that DICOM uses ASCII text with limited precision for many float variables, and this requires adding some tolerance.

neurolabusc added a commit that referenced this pull request Feb 16, 2024
@neurolabusc
Copy link
Collaborator

@BenyAlbatross and @cfhammill can you both see if my latest commit resolves issue 782? To evaluate this, please build the latest version from the development branch:

git clone --branch development https://github.com/rordenlab/dcm2niix.git
cd dcm2niix/console
make
./dcm2niix /path/to/dicoms

@cfhammill
Copy link
Author

Thanks @neurolabusc! I tried the latest development commit and it worked for the case I built the dcm_qa_ct example off of, and worked on 3 other instances of this issue of this problem that I had readily on hand.

Re compiler, that's interesting, I'm using a relatively new GCC to build.

I'm a bit curious about the specific commit, what's the difference between computeGantryTiltPrecise and computeGantryTiltPreciseX, they appear to both be using the new isSameFloatGE

neurolabusc added a commit that referenced this pull request Feb 16, 2024
@neurolabusc
Copy link
Collaborator

The computeGantryTiltPreciseX was just for development and has been removed. I am going to close this PR, as the development branch handles your issue.

@cfhammill
Copy link
Author

thanks. I do want to add one final thought, I don't think it's fair to blame dicom's use of string to represent floating point numbers here, I think the problem must be float error in the computation itself. Even if the vectors themselves are slightly off parallel due to lost precision, there's no reason they would end up with an impossible angle cosine > 1, any precision lost would make them less than exactly parallel with cosine < 1.

Likely not worth changing but there may be more numerically stable versions of the operations used in this calculation.

@cfhammill cfhammill deleted the fix-float-error-gantry-tilt branch February 16, 2024 15:35
yarikoptic added a commit to neurodebian/dcm2niix that referenced this pull request Apr 29, 2024
* tag 'v1.0.20240202': (135 commits)
  Update submodules
  Refactor (rordenlab#791)
  gantry tilt tolerance (rordenlab#791)
  GE step description (rordenlab#790)
  PatientOrient -> Patient Position (rordenlab#786)
  Prevent shell expansion (rordenlab#789)
  Replace presumably accidental bitwise AND operations
  Update JasPer API calls for compatibility with newer versions of the library
  Update divest logic, reducing duplication and supporting new mode of operation
  Fix PhaseEncodingDirectionDisplayed for GE
  Update date
  GE Diffusion issue rordenlab#777 minor
  GE Diffusion issue rordenlab#777
  Kludge for issue 775 (rordenlab#775)
  add docstrings
  better python wrapper I/O
  issue 769: Mildly relax the check for bvec components > 1.
  PRs (rordenlab#745; rordenlab#768)
  Edge cases (rordenlab#763, rordenlab#749)
  Code spell
  ...
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.

2 participants