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

Ability to specify external command to generate acquisition uid #38

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ryansanford
Copy link
Contributor

@ryansanford ryansanford commented Aug 30, 2016

Included in this PR

  • New option uid_ext_command to specify external command to run.
  • Single non-deterministic file from acquisition source files will be used in the command.
  • Non-zero or empty string returned by command will prevent acquisition from being uploaded.
  • Non-zero exit of external command will fail and abort reap attempt

Actions Before Merge

  • Create Proof Of Concept
  • Decide final implementation details
  • Add final implementation details here...
  • Abort and fail reap attempt on external command error

@ryansanford
Copy link
Contributor Author

ryansanford commented Aug 30, 2016

@gsfr
I don't like the current implementation of assuming source dicoms will be in a folder named the same as the zip, however, it was the quickest path to this POC so I didn't have to worry about state set before the zip is created, and after it's created and we have the metadata object. I feel strongly that this option does belong in the dicom_reaper or lower module, and not orthanc_reaper.

How would you feel about passing a set_acq_uid() function down to dcm.pkg_series that could be used if provided? I'm open to other approaches, as my python-foo is still weak.

This is a POC implementation. Consider passing custom function for generation rather than assuming source dicoms are in dir named the same as the resulting zip file. Then the zip meta would be accurate as well.
@gsfr
Copy link
Member

gsfr commented Sep 20, 2016

Rebased and added a new commit. This is untested and likely buggy, but I wanted to show it to you, @ryansanford, for feedback on general approach.

Command line looks like this:

--metadata acquisition "Referring Physician" ReferringPhysicianName

@ryansanford
Copy link
Contributor Author

@gsfr Thanks. I should be able to check this out before I leave.

@ryansanford
Copy link
Contributor Author

@gsfr pushed change to fix an issue with subprocess call. Feel free to refactor to more elegant approach. I only tested that narrow use case. I'll review with a more critical eye tomorrow.

@gsfr
Copy link
Member

gsfr commented Sep 20, 2016

Thanks. I'm going to refactor that just a tad. Also note that I have updated the original description of this PR. I don't think that an empty string return value should prevent reaping. Non-zero exit also doesn't currently prevent reaping, but it probably should.

@gsfr
Copy link
Member

gsfr commented Sep 21, 2016

What is the purpose of the latest commit? Does the previous code not work?

http://stackoverflow.com/q/10563613

@ryansanford
Copy link
Contributor Author

@gsfr I pushed a change so a single file from the series gets passed, rather than the original filename that's no longer valid after it's renamed.

Behavior with a non-zero return from the external command is an empty metadata value and does not prevent the upload. It would be nice to have a defined return/exception interface on pkg_series() so we can fail the operation with predictable results to any callers.

@gsfr
Copy link
Member

gsfr commented Sep 21, 2016

My bad on the first point and agreed on the second point. I can take that on once you agree on the general design and have tested the current implementation.

@ryansanford
Copy link
Contributor Author

Thanks for the python tip @gsfr

I agree with the general design. Cursory testing with static, dicom tag name, and external command is working for me.

👍

@gsfr
Copy link
Member

gsfr commented Oct 20, 2016

@ryansanford I've added basic error handling. Please give it a whirl. We now error out on non-zero return of external command and proceed zero return, even with an empty string.

--metadata acquisition CustomFieldName @true

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