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

[BUGFIX] Move *.dcm Files to data Subdirectory #10

Merged
merged 46 commits into from
Apr 15, 2022

Conversation

adam-grant-hendry
Copy link
Contributor

vtk.vtkDICOMImageReader cannot parse non-dcm files in a folder. To fix this, place all *.dcm files in a separate data subfolder.

@MatthewFlamm
Copy link
Contributor

MatthewFlamm commented Apr 14, 2022

First see comments in pyvista/pyvista#2460. Right now that PR isn't even downloading any data.

But I think you are right that the pyvista downloading functionality does not handle folders well currently. It only works well with a zip file. You may need to zip this data folder in a zip file.

IMO, it is easiest to test this whole thing out using https://docs.pyvista.org/extras/vtk_data.html

You can use your current PR branch to test locally. This means you don't need to have this PR merged to make sure it works right. Just make sure you clear your examples cache before testing.

@akaszynski
Copy link
Member

You may need to zip this data folder in a zip file.

Fully agree. Most efficient to zip the files and certainly easier to download from pyvista.

@adam-grant-hendry
Copy link
Contributor Author

@MatthewFlamm @akaszynski Thank you! Apologies, as this is my first PR. I also notice this PR is trying to add Data/FORGE.omf again. I'll fix this, zip my folder, and test locally.

@akaszynski
Copy link
Member

I think you need to create a new branch from master:

git checkout master
git pull
git checkout -b feat/<name>

@adam-grant-hendry
Copy link
Contributor Author

adam-grant-hendry commented Apr 14, 2022

I think you need to create a new branch from master:

git checkout master
git pull
git checkout -b feat/<name>

When pulling from upstream (pyvista/vtk-data) into my local master, it kept saying I was already up to date when I still had Data/FORGE.omf in my local repo. So I couldn't push an update to my fork that would remove it. I was afraid that if I created a new branch and deleted the old one that this PR would get closed, which happened in #5 .

I decided instead to amend my local master (deleting the file), rebase bugfix/DICOM_Stack on top of it, and force push to my fork. I don't like force pushing (I know it's typically a big no-no). I couldn't get my fork in sync with pyvista/vtk-data, and the screw up was up on my end, so I felt this was the right move. No one else is using my fork, so there's no risk of me screwing up others' work.

Folders cannot be downloaded using the GitHub API, which is causing
DICOM Reader tests to fail in `pyvista`. Instead, zip the folder.
This will be unzipped when downloaded.
@akaszynski
Copy link
Member

Were you planning on zipping this or leaving as is?

@adam-grant-hendry
Copy link
Contributor Author

adam-grant-hendry commented Apr 15, 2022

Were you planning on zipping this or leaving as is?

I zipped it. It's in Data/DICOM_Stack/data.zip.

@adam-grant-hendry
Copy link
Contributor Author

@pyvista/developers @akaszynski Would you please kindly review and let me know if this PR is okay?

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@akaszynski akaszynski merged commit 60f3b2e into pyvista:master Apr 15, 2022
@adam-grant-hendry adam-grant-hendry deleted the bugfix/DICOM_Stack branch April 19, 2022 14:56
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.

None yet

4 participants