pinnacle export 'all' option

[lucserre]

Hi, I know this isn't a typical "issue" per se, but I was wondering if there was a way when using the experimental pinnacle export feature, to have an "all" option for plan and trial, like there is for the 'images' section. At the moment, it seems you have to either accept the default option of selecting the first available plan and trial, or specify them by name. The CLI accepts -i all as input for the images, but requires the name of the plan or trial to export. Just saying, would be handy to have the option to use 'all' for the plan and trial tags too Hope this isn't TOO out of place. thanks.

[lucserre]

Adding some context to above 'request': For centers who are trying to batch convert/export Pinnacle data to DICOM for import into other TPS.

[SimonBiggs]

Hi @lucserre,

I'll let @pchlap take this one once he's a little more available (I think he's under the pump for the next couple of weeks). A key question though, is this something that you'd be willing to attempt implementing as well as writing a test for?

[crcrewso]

@lucserre I'm probably misremembering but I had thought that the system looped over all trials. I'd be happy to help make this work for you as that's exactly our use case too.

[lucserre]

I'd love to be able to help with this kind of thing, but I don't know where to start. I'm happy to try and modify the code - can someone tell me which file I should be looking at?

[SimonBiggs]

Hi @crcrewso,

That'd be brilliant :). Feel free to take point on this.

[crcrewso]

@lucserre Can we start with the basics,

• Do you have a dataset you can share?
• Could you also share the commands you used to convert the dataset?
• What version of pinnacle is this from

I'm trying to remember if this is even a thing in pinnacle, but can a single dataset be used for multiple trials?

[lucserre]

@crcrewso Unfortunately I can't share the datasets (privacy). command used: find ./patients/ -type d -maxdepth 1 -exec pymedphys experimental pinnacle export --mrn -o ./exported/ {} \; This exports only the first plan and trial by default. Various versions of pinnacle are included in our archives, mainly 8.0d and 8.0m for our center. Each patient, can have multiple plans, each referencing a specific dataset (or a repeated existing dataset). Within each plan, you can have multiple trials (in the same way Eclipse lets you have multiple "plans" for a specific series).

[SimonBiggs]

Unfortunately I can't share the datasets (privacy)

Would you be able to make a dummy dataset? Eg, from some physics patients, QA phantoms, or something of that sort?

[pchlap]

Thanks everyone for your contributions to this. I think the CLI would need to be modified from about here to support exporting all plans/trials.

There is some test data here (which is used for the pymedphys automated tests): https://zenodo.org/record/3940117/files/pinnacle_test_data.zip?download=1

There is a dataset on there with multiple plans, but not with multiple trials, unfortunately. If we could get a test set like that with multiple trials that would be ideal to develop this.

Thanks @crcrewso for guiding this. If needed I'd be happy to jump in and contribute.

[lucserre]

https://ln5.sync.com/dl/e580fd7e0/txbhgtj4-kahjzfd7-yxrxxv2p-i3bhy53a A plan with three trials on a pixie phantom. Just for information, third trial has no dose saved.

[crcrewso]

Thank you, the samples are a huge help @pchlap I can see two ways of accomplishing this from the point in the code you suggested.

1. Add a flag and only process all trials with the flag
2. Change the default behaviour to always handle multiple trials (with or without a flag to revert to the previous behaviour)

Do you have any preferences?

[lucserre]

In my opinion: Without -p or -t flags, the default behavior should be to export all trials from within all plans, as opposed to the 'first' one as currently coded. If -p is specified, export all the trials within that plan. If -p and -t are specified, export that specific trial from that specific plan.

[sjswerdloff]

In my opinion: Without -p or -t flags, the default behavior should be to export all trials from within all plans, as opposed to the 'first' one as currently coded. If -p is specified, export all the trials within that plan. If -p and -t are specified, export that specific trial from that specific plan.

In general I believe that it is best to not break with a previous interface without strong justification. Extending by adding flags or arguments that doesn't break previous usage is my preferred strategy (in all my software development, FOSS or commercial). Others may have calling scripts based on the behaviour with the current interface. Changing the meaning of something can cause very significant grief.

[SimonBiggs]

While I agree, part of the understanding behind "experimental" is that API breakages are more allowed. If there is a good reason to make the change, within experimental is the time to do it...

But essentially I am agreeing with you. It shouldn't be done without strong justification in general. Just that part of the justification that can help make it more strong is that it is within experimental.

[lucserre]

I believe a good justification for changing the behavior in this case, is that default behavior in it's current configuration is flawed. (in my opinion, and please no offense intended - I recognize it's experimental!) Exporting the 'first' plan and 'first' trial is likely never what any user actually wants to do, rather this would be a special case. When someone works on a Pinnacle plans and trials, the 2nd and 3rd trials area usually the more mature versions of their attempts and by the time they have a good clinical plan to use, they maybe don't even care about their first attempts at a good trial. If anything, defaulting to the last trial would make more sense (I'm not advocating this). I believe the common use cases would be:

• user wants to export everything; or
• user know what they want to export

I don't think anyone would ever use it this way:

• user wants to export only the first trial in the first plan

Setting Default behavior to exporting all plans/trials is a safe default, since it will likely satisfy every user. If the user doesn't want to export all the data, then they can either delete the exported plans/trials they don't want, or, specify what they want with flags.

Thanks everyone

[sjswerdloff]

I believe a good justification for changing the behavior in this case, is that default behavior in it's current configuration is flawed. (in my opinion, and please no offense intended - I recognize it's experimental!) Exporting the 'first' plan and 'first' trial is likely never what any user actually wants to do, rather this would be a special case. When someone works on a Pinnacle plans and trials, the 2nd and 3rd trials area usually the more mature versions of their attempts and by the time they have a good clinical plan to use, they maybe don't even care about their first attempts at a good trial. If anything, defaulting to the last trial would make more sense (I'm not advocating this). I believe the common use cases would be:

• user wants to export everything; or
• user know what they want to export

I don't think anyone would ever use it this way:

• user wants to export only the first trial in the first plan

Setting Default behavior to exporting all plans/trials is a safe default, since it will likely satisfy every user. If the user doesn't want to export all the data, then they can either delete the exported plans/trials they don't want, or, specify what they want with flags.

Thanks everyone

That is strong justification for a change. Sold...

[SimonBiggs]

no offense intended

Hopefully no offense ever :). At the end of the day iron sharpens iron and by getting real feedback at the end of the day it can be made better for everyone :)

[pchlap]

Hi all, yep I would agree with what is being said here. To be honest this was never really given much thought. As @lucserre mentions usually you would want to export a specific plan/trial, its unlikely that the first one is what you want. When I use the tool I always export a trial named FINAL, since thats the convention at our centre.

I think exporting all by default is good. One option could be that the -p and -t option accepts a regular expression with a default value of "*". Then any plan/trial that matches the regex would be exported. However I don't want to overcomplicate things so perhaps just have all plans/trials exported when -p/-t aren't set, and when they are use the current behavior.

[SimonBiggs]

I am happy to call it a consensus. @crcrewso and @lucserre you're free to create a PR using your discretion which of the two options @pchlap mentioned above. I suspect for now going the simpler of the two approaches would be best.

[crcrewso]

I just submitted PR #1637 as an expedient solution to this problem. I do intend to properly refactor the code, but felt that @lucserre has been waiting too long.

[lucserre]

@crcrewso Thank you for the expediency! Definitely nothing is owed to me, so keeping me waiting shouldn't be a concern. Unfortunately, I am away from the office, and unable to test this code until Monday. I'll definitely report back when I do. Thanks again.

[lucserre]

Any tips on how I can 'try' this mortification? I've git cloned the code locally, but not sure how to work with this source code (I installed pymedphys with pip).

[SimonBiggs]

Hi @lucserre,

The PR with this fix is over at:

When you go there you can see that the code itself for the fix is over at @crcrewso's fork:

Clicking on crcrewso:cc-all-trials takes you to his code:

Clicking Code on his fork gives you the following:

Meaning you can clone his fork locally by running:

git clone https://github.com/crcrewso/pymedphys.git

To then access the branch for his PR you need to run:

git checkout cc-all-trials

Then, to install pymedphys with that code, you run:

pip install -e .[user]

Let us know how you go :slightly_smiling_face:

[crcrewso]

Hi @lucserre,

The PR with this fix is over at:

When you go there you can see that the code itself for the fix is over at @crcrewso's fork:

Clicking on crcrewso:cc-all-trials takes you to his code:

Clicking Code on his fork gives you the following:

Meaning you can clone his fork locally by running:

git clone https://github.com/crcrewso/pymedphys.git

To then access the branch for his PR you need to run:

git checkout cc-all-trials

Then, to install pymedphys with that code, you run:

pip install -e .[user]

Let us know how you go :slightly_smiling_face:

@SimonBiggs Thank you for typing that up so quickly! I'll remember to include instructions like that in the future for situations like this.

[lucserre]

Hi! I know it's been a while, but I'm just getting to this now. Is this PR still the best way to test out these new features or have they been incorporated into the main program now? (sorry, don't know the right lingo) Thanks

[pchlap]

Hi @lucserre, it's been a while since I have looked at this but it seems the previous PR has been closed. So it would probably be best to open up a new one but it can be based on the same branch/fork as before.

[lucserre]

After trying the modification, I ended up getting the following errors:

/home/physics/venvs/pymedphys_pr/lib/python3.8/site-packages/pydicom/dataset.py:2147: UserWarning: Camel case attribute 'ReferringPhysiciansName' used which is not in the element keyword data dictionary
  warnings.warn(msg)
/home/physics/venvs/pymedphys_pr/lib/python3.8/site-packages/pydicom/dataset.py:2147: UserWarning: Camel case attribute 'ManufacturersModelName' used which is not in the element keyword data dictionary
  warnings.warn(msg)
2023-01-18 14:57:18 INFO     Exporting point: tattoos
2023-01-18 14:57:18 INFO     Exporting point: isocentre
2023-01-18 14:57:18 INFO     Exporting point: norm point
Traceback (most recent call last):
  File "/home/physics/venvs/pymedphys_pr/bin/pymedphys", line 8, in <module>
    sys.exit(main())
  File "/home/physics/venvs/pymedphys_pr/lib/python3.8/site-packages/pymedphys/cli/__init__.py", line 148, in pymedphys_cli
    args.func(args)
  File "/home/physics/venvs/pymedphys_pr/lib/python3.8/site-packages/pymedphys/_experimental/pinnacle/pinnacle_cli.py", line 258, in export_cli
    p.export_struct(plan=plan, export_path=output_directory)
  File "/home/physics/venvs/pymedphys_pr/lib/python3.8/site-packages/pymedphys/_experimental/pinnacle/pinnacle.py", line 206, in export_struct
    convert_struct(plan, export_path, skip_pattern)
  File "/home/physics/venvs/pymedphys_pr/lib/python3.8/site-packages/pymedphys/_experimental/pinnacle/rtstruct.py", line 538, in convert_struct
    ds = read_points(ds, plan)
  File "/home/physics/venvs/pymedphys_pr/lib/python3.8/site-packages/pymedphys/_experimental/pinnacle/rtstruct.py", line 137, in read_points
    roi_contour.ROIDisplayColor = colors[point["Color"]]
KeyError: 'inverse_grey'

[crcrewso]

KeyError: 'inverse_grey'

This is the problem, the definition of inverse_grey needs to be added to: https://github.com/pymedphys/pymedphys/blob/0b51f771f7bbebc42aa7f5ca984775ab747e4b8a/lib/pymedphys/_experimental/pinnacle/constants.py#L54

Sorry I don't think there's current support for undefined colors

May I suggest colors["inverse_grey"] = ["149", "147", "145"]