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

[BUG] Fix marching cubes not in the right space when CT is not aligned #25

Merged
merged 21 commits into from
Dec 6, 2023

Conversation

alexrockhill
Copy link
Collaborator

@alexrockhill
Copy link
Collaborator Author

@larsoner would you happen to know why these tests are failing again?

It might be

Collecting git+https://github.com/pyvista/pyvista@main
  Cloning https://github.com/pyvista/pyvista (to revision main) to /tmp/pip-req-build-d27__lhw
  Running command git clone --filter=blob:none --quiet https://github.com/pyvista/pyvista /tmp/pip-req-build-d27__lhw
  Resolved https://github.com/pyvista/pyvista to commit 098a6beabb4ccc924a789ede86002d9fdf56a8aa
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Collecting PyQt6
  Downloading PyQt6-6.6.1-cp38-abi3-manylinux_2_28_x86_64.whl (7.9 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 7.9/7.9 MB 77.7 MB/s eta 0:00:00

Collecting PyQt6-Qt6!=6.6.1
  Downloading PyQt6_Qt6-6.6.0-py3-none-manylinux_2_28_x86_64.whl (67.4 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 67.4/67.4 MB 38.5 MB/s eta 0:00:00

the PyQt version mismatch here but that's odd that it worked for a bit and then stopped working... I'll try with PyQt6 != 6.6.1

@alexrockhill
Copy link
Collaborator Author

Ok, this should handle when images are not LIA now by going through scanner RAS. I also added some very standard boilerplate to the webpage since it was looking very lonely. If there's any issue with that I can split it out. Otherwise, looks good to go.

@alexrockhill
Copy link
Collaborator Author

Well there was actually a larger issue where all the tests were skipped because they didn't have imageio-ffmpeg... there weren't that many failures so it wasn't too hard to fix.

@alexrockhill
Copy link
Collaborator Author

Okay to merge by me

@alexrockhill
Copy link
Collaborator Author

Oh man well I looked again and the 3D rendering of electrodes when the CT is not aligned were off so then I had to think about with no recon-all what space the 3D rendering should be in. On main it's in surface RAS but if it's surface RAS of an unaligned CT this all just seems so unnecessarily complicated so I streamlined the workflow a bit more:

  • The base image is loaded
    • If it is aligned to a recon, show the brain and head surfaces in scanner RAS (aligned)
    • Else just do marching cubes on the image and also show that in scanner RAS
  • Electrode positions are in head, convert to mri (surface RAS) of base image using trans then convert to scanner RAS

This way everything internally is in scanner RAS and I think it just simplifies a lot all these transforms which caused this bug.

@alexrockhill alexrockhill marked this pull request as draft December 5, 2023 22:06
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way everything internally is in scanner RAS and I think it just simplifies a lot all these transforms which caused this bug.

Agreed sometimes surface RAS is a pain, and scanner RAS is what nibabel uses everywhere so if it makes stuff easier to work in scanner RAS it's a reasonable choice!

Feel free to merge if you're happy, just one comment about 1.6

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@alexrockhill alexrockhill marked this pull request as ready for review December 6, 2023 01:05
@alexrockhill
Copy link
Collaborator Author

Ok, thanks for the review, the last commit just finishes moving the 3D surfaces to scanner RAS, good that you approve of doing that.

Sounds good, I'll wait a day or so in case I hear back from the people who reported the bug if they have a chance to test this before merging.

Re 1.6, it's odd that it fails on stable still. I guess it's fine just to wait to move to stable until 1.7...

@alexrockhill
Copy link
Collaborator Author

All the examples look the same: https://output.circle-artifacts.com/output/job/aea15c4c-5071-434b-832d-f56b4ea42749/artifacts/0/dev/auto_examples/index.html

The diff is a bit large to review but will merge tomorrow unless I come upon any other issues merging this into the other two WIP PRs.

@alexrockhill alexrockhill merged commit 7e90930 into mne-tools:main Dec 6, 2023
15 checks passed
@alexrockhill alexrockhill deleted the bug branch December 6, 2023 17:59
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