search

ImagingDataCommons / highdicom

High-level DICOM abstractions for the Python programming language
https://highdicom.readthedocs.io
MIT License
164 stars 34 forks source link

#231 convert plane position sequences to hd.PlanePositionSlideSequence #232

Closed thomas-albrecht closed 1 year ago

thomas-albrecht commented 1 year ago

This is to fix the Issue #231 that I created myself.

thomas-albrecht commented 1 year ago

Even on the unchanged version that I cloned a lot of tests fail.

That was fixed by limiting the pydicom version.

CPBridge commented 1 year ago

Hi @thomas-albrecht thanks a lot for reporting this bug and implementing the fix with tests, really appreciate the help.

However, can you please remove your change to the setup.py. I am aware of the issue with pydicom 2.4 and will look into it soon. It is a much larger issue that this pull requests and I will fix it in another PR. If your tests pass with pydicom==2.3.1 I am satisfied, don't try and make them pass with 2.4 just yet

CPBridge commented 1 year ago

c.f. #233

thomas-albrecht commented 1 year ago

Hi @CPBridge, sure I can remove the setup.py thing.

I did not actually try to make the tests pass with pydicom 2.4. I just specified the version of pydicom that highdicom currently works with: pydicom>=2.3.0,<2.4 because 2.4 does not work yet.

As it is without this change, if you just clone and install it, most tests will fail. It's good to hear though that you are looking into making that work separately.

Edit: I just saw now that it seems to be fixed in pydicom 2.4.1 so that change to setup.py is really superfluous.

CPBridge commented 1 year ago

Thanks @thomas-albrecht . For reference, there were various issues introduced by pydicom 2.4.0. I currently have two PRs on highdicom waiting to be merged to fix this: #239 and #238. Once those are merged, pipelines on this PR should run correctly.

I'm approving this but may wait to merge it until other things are sorted out

CPBridge commented 1 year ago

Hi @thomas-albrecht, highdicom's master branch is now in a working state again after the chaos of the pydicom 2.4.0 release. Could you please rebase your branch on master, which should get tests working on this branch again?

Furthermore after running the github actions I noticed that there are some linter errors that I'm hoping you might be able to fix. Thanks!

thomas-albrecht commented 1 year ago

Hi @CPBridge, I am a bit embarrassed. I obviously don't know how to use git rebase. I tried to rebase to the master branch but now my branch contains hundreds of old commits and shows 42 changed files.

I just did a manual rebase by starting from scratch in the pull request https://github.com/ImagingDataCommons/highdicom/pull/240 and will close this one.