add DICOMReader #2460

adam-grant-hendry · 2022-04-13T04:34:06Z

Overview

Add DICOMReader (i.e. _vtk.vtkDICOMImageReader) to utilities/reader.py. This is commit will assist in the bugfix commit for Issue #500 (NOTE: This originated on the old pyvista/support issue tracker).

Details

Add DICOMReader to standard dataset readers
Add download_dicom_stack to examples/downloads.py

(added by @akaszynski)
Resolves #2467 as well.

DICOM dataset was altered as is no longer a pancreas scan. Rather than explicitly name the type of scan, simply refer to it as a dicom stack, which is what is important, in case the data needs to change.

`pyvista/utilities/reader/get_reader` requires a file extension to work (it does not support folders). Since we would have to specify explicitly the files to be read from teh folder, we call the reader directly. However, we still support `get_reader` for reading a single `.dcm` file.

`pyvista/vtk-data` PR #5 was accepted. Pull dicom stack from here instead of temporary local directory.

github-actions

🚀 Thanks for opening your first pull request in PyVista! 🚀 Please make sure you read (skim 😉) our Contributing Guide and abide by our Code of Conduct.
A few things to keep in mind:

Remember to run the tests locally to make debugging issues easier.
If you need help writing tests, take a look at the existing ones for inspiration. If you do not know where to start, let @pyvista/developers know and we will walk you through it.
All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
No matter what, we are really grateful that you put in the effort to open this PR! ⭐

MatthewFlamm

Looking good so far.

pyvista/examples/downloads.py

pyvista/utilities/reader.py

tests/utilities/test_reader.py

akaszynski · 2022-04-14T13:40:40Z

See the notes regarding Style checking with pyvista.

akaszynski · 2022-04-14T13:40:59Z

@adam-grant-hendry, please let me know if you need help cleaning up this PR. I might have time later this week.

adam-grant-hendry · 2022-04-15T02:16:34Z

See the notes regarding Style checking with pyvista.

Yes, I was having trouble with pre-commit. I submitted a bug to check-jsonschema that got corrected. After I updated .pre-commit-config.yaml to the latest version and now things are working properly:

❯ pre-commit run --all-files
[INFO] Initializing environment for https://github.com/python-jsonschema/check-jsonschema.
[INFO] Installing environment for https://github.com/python-jsonschema/check-jsonschema.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
black....................................................................Passed
isort....................................................................Passed
flake8...................................................................Passed
codespell................................................................Passed
pydocstyle...............................................................Passed
mypy.....................................................................Passed
check for merge conflicts................................................Passed
debug statements (python)................................................Passed
don't commit to branch...................................................Passed
Validate GitHub Workflows................................................Passed

- Use `_download_and_read` for `dicom_stack` - Add volume plot in Examples docstrings - Update `DICOMReader` to accept files or folders - Remove redundant `dir_path` assignment in `examples.py` - Add test for valid mesh after loading single *.dcm file - Add test to `test_examples.py` - Update `check-jsonschema` to latest version

adam-grant-hendry · 2022-04-15T03:01:01Z

@adam-grant-hendry, please let me know if you need help cleaning up this PR. I might have time later this week.

Yes please! That would be great. Currently tests are failing. One reason is vtk-data PR #10 with the zip file needs to be merged. Another is the docs:

I've attached the stdout and stderr from trying to build the docs with:

❯ cd doc
❯ py -m sphinx -M html . _build > win_build_docs_stdout.txt 2> win_build_docs_stderr.txt

I've attached the output from trying to run doctest:

❯ py -m pytest -v --doctest-modules pyvista > doctest_stdout.txt

It looks like I don't have a lot of GDAL dlls...

win_build_docs_stderr.txt
win_build_docs_stdout.txt
doctest_stdout.txt

codecov · 2022-04-15T18:01:57Z

Codecov Report

Merging #2460 (21a5d5d) into main (17ed849) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2460      +/-   ##
==========================================
- Coverage   93.73%   93.70%   -0.03%     
==========================================
  Files          75       75              
  Lines       16052    16073      +21     
==========================================
+ Hits        15046    15062      +16     
- Misses       1006     1011       +5

akaszynski · 2022-04-16T06:03:42Z

1e97ace should take care of CI issues.

Really pleased with this. Thanks for adding it @adam-grant-hendry, looks great!

MatthewFlamm

Three very minor comments/suggestions to take or leave. The other is more important IMO.

pyvista/examples/downloads.py

pyvista/utilities/reader.py

Co-authored-by: MatthewFlamm <39341281+MatthewFlamm@users.noreply.github.com>

akaszynski

Calling this one good to go, will wait 24H to merge for any final comments.

akaszynski · 2022-04-18T16:03:06Z

Congrats on your first PR @adam-grant-hendry! Keep'em coming.

pyvista/utilities/reader.py

njneeteson · 2022-04-18T19:15:26Z

Perhaps I'm misunderstanding the changes/additions, but is it still the case that to actually read a DICOM directory you will need to specifically/manually import the DICOM reader sub-class and still cannot use pyvista.read? Since the read function uses a dictionary to map to a specific reader using the given file extension, it seems like you would have to give a *.dcm type input to get the DICOM reader subclass and then end up in "file-reading mode" and only get a single slice rather than the whole directory. Unless I'm missing something the only way to actually use it in "directory-reading mode" will be to specifically import it and then give it a directory path as an input?

Maybe the solution here is to add an optional flag kwarg to the BaseReader class to have it use the file extension to select a sub-class from the dictionary but if the flag is set, it then chops off the extension right before _set_filename_or_directory? Then you could pass one of the DICOM files to have it auto-select the reader but with read_dir=True or something like that to have it parse the whole directory that file is in rather than just the file? If there's other readers that look at directories it could be useful for them as well.

adam-grant-hendry · 2022-04-18T19:39:14Z

Perhaps I'm misunderstanding the changes/additions, but is it still the case that to actually read a DICOM directory you will need to specifically/manually import the DICOM reader sub-class and still cannot use pyvista.read? Since the read function uses a dictionary to map to a specific reader using the given file extension, it seems like you would have to give a *.dcm type input to get the DICOM reader subclass and then end up in "file-reading mode" and only get a single slice rather than the whole directory. Unless I'm missing something the only way to actually use it in "directory-reading mode" will be to specifically import it and then give it a directory path as an input?

Maybe the solution here is to add an optional flag kwarg to the BaseReader class to have it use the file extension to select a sub-class from the dictionary but if the flag is set, it then chops off the extension right before _set_filename_or_directory? Then you could pass one of the DICOM files to have it auto-select the reader but with read_dir=True or something like that to have it parse the whole directory that file is in rather than just the file? If there's other readers that look at directories it could be useful for them as well.

Yes, to read a a stack of DICOM images, you must import the reader directly and pass it the folder (folders don't have file extensions). For a single DICOM, you could use the reader.

This begs the question again regarding checking file extensions though...The entire reader is based solely off reading file extensions to determine the file type...I think we should update our docs to clearly state supported file types for readers.

@akaszynski @adeak @MatthewFlamm @njneeteson I think we should keep #2471 open. If a file extension changes, the reader breaks.

njneeteson · 2022-04-18T19:55:26Z

With respect to #2471 I just don't think it makes sense. As I commented in that issue, there's two possibilities: (1) You used pyvista.read (EDIT: pyvista.get_reader actually) and ended up there automatically so the "suffix checking" already happened and doesn't need to be implemented at the sub-class level, or (2) you imported a specific reader subclass and passed it a non-conforming filename because you know what you're doing.

The relevant extensions for each sub-class reader are also given in the docstring for each subclass. E.g. the doc page for pyvista.PVDReader says:

PVD Reader for .pvd files.

@akaszynski @adeak @MatthewFlamm @njneeteson I think we should keep #2471 open. If a file extension changes, the reader breaks.

For this reason I don't really bother with pv.read, I just use VTK to import images and then wrap the output. I'm not really sure what can be done about it though. It seems to be sufficiently documented to me and I don't know how you could really modify the way the pyvista reader works to get around it without making it significantly more complicated.

EDIT: I dug into utilities/fileio.py:read and now I am very confused about how pyvista.read even works and how the stuff in utilities/reader.py is related. Is it just totally separate?

MatthewFlamm · 2022-04-18T20:03:45Z

We need to separate a few things. Today pyvista.read is totally separate from the Reader classes. I'd like to unify them in a future PR.

You can use pyvista.get_reader to choose a Reader based on extension. If your extension is non standard, e.g. a directory, then you need to instantiate a Reader class explicitly.

If the specific Reader has special requirements, we can override methods as needed. Sometimes vtk will throw warnings, but will still read non complying files OK

njneeteson · 2022-04-18T20:16:45Z

We need to separate a few things. Today pyvista.read is totally separate from the Reader classes. I'd like to unify them in a future PR.

You can use pyvista.get_reader to choose a Reader based on extension. If your extension is non standard, e.g. a directory, then you need to instantiate a Reader class explicitly.

If the specific Reader has special requirements, we can override methods as needed. Sometimes vtk will throw warnings, but will still read non complying files OK

Yeah I see where I am confused now. I had assumed that pv.read hooked into the classes in the reader utility file. It's particularly confusing because pv.read has a force_ext kwarg, which it seems to me would be useful in BaseReader but is obviously not there. However perhaps there's not much point in building on either solution incrementally with stuff like that if eventually someone is going to have to figure out how to unify the two different file reading approaches.

akaszynski · 2022-04-18T20:42:53Z

We need to separate a few things. Today pyvista.read is totally separate from the Reader classes. I'd like to unify them in a future PR.

Agreed, and that's out of the scope of this (already long) PR.

MatthewFlamm

I'm happy with this now, one last comment for consideration, but I think it is okay to merge as is.

pyvista/utilities/reader.py

adam-grant-hendry · 2022-04-18T23:53:37Z

Congrats on your first PR @adam-grant-hendry! Keep'em coming.

Thanks @akaszynski !! I definitely will! I think next up I'd love to work on documentation and add some more examples!

adam-grant-hendry · 2022-04-19T00:26:16Z

As I commented in that issue, there's two possibilities: (1) You used pyvista.read (EDIT: pyvista.get_reader actually) and ended up there automatically so the "suffix checking" already happened and doesn't need to be implemented at the sub-class level, or (2) you imported a specific reader subclass and passed it a non-conforming filename because you know what you're doing.

Totally fair. As long as things are documented so the end user doesn't go "What the heck?!", I like this approach (assume your user is smart).

njneeteson · 2022-04-19T02:10:30Z

As I commented in that issue, there's two possibilities: (1) You used pyvista.read (EDIT: pyvista.get_reader actually) and ended up there automatically so the "suffix checking" already happened and doesn't need to be implemented at the sub-class level, or (2) you imported a specific reader subclass and passed it a non-conforming filename because you know what you're doing.

Totally fair. As long as things are documented so the end user doesn't go "What the heck?!", I like this approach (assume your user is smart).

I think if someone was using pyvista and somehow was in a situation where get_reader can't give them the right reader because their extensions are messed up and they don't actually know what the format of their data is so they don't know what specific reader to use, they're in a lot of trouble and I'm not really sure how we can preemptively help them via either documentation or code :)

pyvista/utilities/reader.py

adeak · 2022-04-20T19:15:23Z

I'm getting the following error from the docstring examples of pyvista.examples.downloads.download_dual_sphere_animation during local doc builds from main:

/home/adeak/pyvista/pyvista/examples/downloads.py:docstring of pyvista.examples.downloads.download_dual_sphere_animation:27: WARNING: Exception occurred in plotting pyvista-examples-downloads-download_dual_sphere_animation-1
 from /home/adeak/pyvista/doc/api/examples/_autosummary/pyvista.examples.downloads.download_dual_sphere_animation.rst:
Traceback (most recent call last):
  File "/home/adeak/pyvista/pyvista/ext/plot_directive.py", line 315, in _run_code
    exec(code, ns)
  File "<string>", line 4, in <module>
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 1420, in __init__
    self._set_filename(filename)
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 1471, in _set_filename
    self._update_information()
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 1488, in _update_information
    self._parse_file()
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 1511, in _parse_file
    self.set_active_time_value(self.time_values[0])
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 1531, in set_active_time_value
    self._active_readers = [
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 1532, in <listcomp>
    get_reader(os.path.join(self.__directory, dataset.path))
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 103, in get_reader
    return Reader(filename)
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 130, in __init__
    self.path = path
  File "/home/adeak/pyvista/pyvista/utilities/reader.py", line 211, in path
    raise FileNotFoundError(f"Path '{path}' is invalid or does not exist.")
FileNotFoundError: Path '/home/adeak/.local/share/pyvista/examples/dualSphereAnimation/dualSphereAnimation_P00T0000.vtp' is invalid or does not exist.

It looks as if it tried using a cached file (which I don't have) and not falling back to download. Any idea what's off? Or are we enforcing local caches for doc building and I have to run that function separately first?

akaszynski · 2022-04-20T20:11:06Z

You'll have to clear out your cache. The issue is (and I should have addressed this), is we were improperly downloading a directory from github. The new example works now, but we need to flush the cache, or delete that invalid file.

adeak · 2022-04-20T20:16:13Z

I see, so the cached version is the broken one, and what we have now works. This should probably only affect at most a few pyvista devs and nobody else, so this is fine then. Thanks @akaszynski!

adeak · 2022-04-20T20:52:56Z

Actually @akaszynski which cache do you mean exactly? I don't have the missing directory in `~/.local/share/pyvista/examples`, which is exactly why I get the error. And I've been running `make -C doc clean html`, which should purge everything.
I've grepped for the "not found" filename in my repo (with find rather than git grep) and there was a single hit from ./doc/_build/doctrees/api/examples/_autosummary/pyvista.examples.downloads.download_dual_sphere_animation.doctree which I can confirm is wiped by make clean. The rst file mentioned at the start of the traceback is at ./doc/api/examples/_autosummary/pyvista.examples.downloads.download_dual_sphere_animation.rst which also gets purged on clean.

~~What am I doing wrong?~~

(I've also deleted ~/.local/share/pyvista/examples/dualSphere* now, although those files seem to have different names. Let me see how the doc build works now.)

Edit: that was it, thanks.

akaszynski · 2022-04-21T01:11:23Z

Edit: that was it, thanks.

Was going to respond, but been in JS land for the past two days and it's getting intense. Finally getting around to fixing pythreejs resize.

adamgranthendry and others added 5 commits April 11, 2022 15:22

Add DICOMReader and pancreas example

4127fb8

Add test_dcmreader

02e1cae

Change download and test to DICOM stack

079a2ef

DICOM dataset was altered as is no longer a pancreas scan. Rather than explicitly name the type of scan, simply refer to it as a dicom stack, which is what is important, in case the data needs to change.

Update download_dicom_stack to pull from VTK_DATA_PATH

e94f8a5

`pyvista/vtk-data` PR #5 was accepted. Pull dicom stack from here instead of temporary local directory.

github-actions bot reviewed Apr 13, 2022

View reviewed changes

tkoyama010 added the enhancement Changes that enhance the library label Apr 13, 2022

Fix pre-commit errors

684bddd

MatthewFlamm reviewed Apr 14, 2022

View reviewed changes

MatthewFlamm mentioned this pull request Apr 14, 2022

[BUGFIX] Move *.dcm Files to data Subdirectory pyvista/vtk-data#10

Merged

adam-grant-hendry force-pushed the feat/dicom_reader branch from b597b64 to f6b0c85 Compare April 15, 2022 02:25

Merge branch 'main' into feat/dicom_reader

4b694c2

cleanup

1e97ace

MatthewFlamm reviewed Apr 16, 2022

View reviewed changes

pyvista/examples/downloads.py Outdated Show resolved Hide resolved

pyvista/utilities/reader.py Outdated Show resolved Hide resolved

pyvista/utilities/reader.py Outdated Show resolved Hide resolved

pyvista/utilities/reader.py Outdated Show resolved Hide resolved

akaszynski mentioned this pull request Apr 16, 2022

Add reader extension checks #2471

Closed

akaszynski and others added 3 commits April 16, 2022 08:02

Update pyvista/examples/downloads.py

859b196

Co-authored-by: MatthewFlamm <39341281+MatthewFlamm@users.noreply.github.com>

cleanup common methods

abcd8aa

small test cleanup

1e14877

akaszynski changed the title ~~Feat/dicom_reader~~ add DICOMReader Apr 17, 2022

akaszynski added 3 commits April 17, 2022 21:30

remove extension check; reset cache

ae03e23

remove warning test

23cfcc0

improve coverage

e1bf086

akaszynski approved these changes Apr 18, 2022

View reviewed changes

akaszynski mentioned this pull request Apr 18, 2022

Set default scalars for filters #2474

Merged

MatthewFlamm reviewed Apr 18, 2022

View reviewed changes

pyvista/utilities/reader.py Show resolved Hide resolved

pyvista/utilities/reader.py Outdated Show resolved Hide resolved

njneeteson reviewed Apr 18, 2022

View reviewed changes

pyvista/utilities/reader.py Outdated Show resolved Hide resolved

remove __init__ in DICOMReader

7af391c

MatthewFlamm approved these changes Apr 18, 2022

View reviewed changes

pyvista/utilities/reader.py Outdated Show resolved Hide resolved

refactor filename --> path

2c01008

akaszynski enabled auto-merge (squash) April 19, 2022 03:06

final cleanup

345dd61

njneeteson reviewed Apr 19, 2022

View reviewed changes

pyvista/utilities/reader.py Outdated Show resolved Hide resolved

akaszynski disabled auto-merge April 19, 2022 04:04

reset directory and filename when setting filename and directory

21a5d5d

akaszynski enabled auto-merge (squash) April 19, 2022 04:06

akaszynski merged commit 8179107 into pyvista:main Apr 19, 2022

adam-grant-hendry deleted the feat/dicom_reader branch April 19, 2022 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add DICOMReader #2460

add DICOMReader #2460

adam-grant-hendry commented Apr 13, 2022 •

edited by akaszynski

github-actions bot left a comment

MatthewFlamm left a comment

akaszynski commented Apr 14, 2022

akaszynski commented Apr 14, 2022

adam-grant-hendry commented Apr 15, 2022

adam-grant-hendry commented Apr 15, 2022

codecov bot commented Apr 15, 2022 •

edited

akaszynski commented Apr 16, 2022

MatthewFlamm left a comment •

edited

akaszynski left a comment

akaszynski commented Apr 18, 2022

njneeteson commented Apr 18, 2022 •

edited

adam-grant-hendry commented Apr 18, 2022 •

edited

njneeteson commented Apr 18, 2022 •

edited

MatthewFlamm commented Apr 18, 2022

njneeteson commented Apr 18, 2022

akaszynski commented Apr 18, 2022

MatthewFlamm left a comment

adam-grant-hendry commented Apr 18, 2022

adam-grant-hendry commented Apr 19, 2022

njneeteson commented Apr 19, 2022 •

edited

adeak commented Apr 20, 2022 •

edited

akaszynski commented Apr 20, 2022

adeak commented Apr 20, 2022

adeak commented Apr 20, 2022 •

edited

akaszynski commented Apr 21, 2022

add DICOMReader #2460

add DICOMReader #2460

Conversation

adam-grant-hendry commented Apr 13, 2022 • edited by akaszynski

Overview

Details

github-actions bot left a comment

Choose a reason for hiding this comment

MatthewFlamm left a comment

Choose a reason for hiding this comment

akaszynski commented Apr 14, 2022

akaszynski commented Apr 14, 2022

adam-grant-hendry commented Apr 15, 2022

adam-grant-hendry commented Apr 15, 2022

codecov bot commented Apr 15, 2022 • edited

Codecov Report

akaszynski commented Apr 16, 2022

MatthewFlamm left a comment • edited

Choose a reason for hiding this comment

akaszynski left a comment

Choose a reason for hiding this comment

akaszynski commented Apr 18, 2022

njneeteson commented Apr 18, 2022 • edited

adam-grant-hendry commented Apr 18, 2022 • edited

njneeteson commented Apr 18, 2022 • edited

MatthewFlamm commented Apr 18, 2022

njneeteson commented Apr 18, 2022

akaszynski commented Apr 18, 2022

MatthewFlamm left a comment

Choose a reason for hiding this comment

adam-grant-hendry commented Apr 18, 2022

adam-grant-hendry commented Apr 19, 2022

njneeteson commented Apr 19, 2022 • edited

adeak commented Apr 20, 2022 • edited

akaszynski commented Apr 20, 2022

adeak commented Apr 20, 2022

adeak commented Apr 20, 2022 • edited

akaszynski commented Apr 21, 2022

adam-grant-hendry commented Apr 13, 2022 •

edited by akaszynski

codecov bot commented Apr 15, 2022 •

edited

MatthewFlamm left a comment •

edited

njneeteson commented Apr 18, 2022 •

edited

adam-grant-hendry commented Apr 18, 2022 •

edited

njneeteson commented Apr 18, 2022 •

edited

njneeteson commented Apr 19, 2022 •

edited

adeak commented Apr 20, 2022 •

edited

adeak commented Apr 20, 2022 •

edited