Change the default global_normalisation from the maximum value of dose_reference to dose_evaluation

[guanfada]

line 92, and line 253, change "dose_reference" to "dose_evaluation" as the default global_normalisation

---

Edit by @SimonBiggs

See #1603 (comment) for reasoning

[SimonBiggs]

@Matthew-Jennings, do you have any thoughts on this change? Do you happen to know what other Gamma software has implemented by default?

[SimonBiggs]

@guanfada are you able to track down where the following test is within PyMedPhys and update the baseline value now that the default normalisation has changed:

https://github.com/pymedphys/pymedphys/runs/4636766556?check_suite_focus=true#step:39:246

[SimonBiggs]

@pchlap might you have the chance to have a look at why the entire file is set as lines changed:

https://github.com/pymedphys/pymedphys/pull/1604/files

I suspect potentially there are different line endings being used. In that case we should add the following pre-commit hook to enforce lf line endings:

https://github.com/pre-commit/pre-commit-hooks/pull/233/files

What do you think?

[sjswerdloff]

@pchlap might you have the chance to have a look at why the entire file is set as lines changed:

https://github.com/pymedphys/pymedphys/pull/1604/files

I suspect potentially there are different line endings being used. In that case we should add the following pre-commit hook to enforce lf line endings:

https://github.com/pre-commit/pre-commit-hooks/pull/233/files

What do you think?

I think it's a good idea.
But I also think diff tools should be configurable to ignore this particular difference as well.
For languages that don't have semantics associated with 'white space', diff tools should be configurable to ignore that entirely.

I managed to approve the referenced pre commit PR without realising it was someone else's project. I have not looked at the PR here other than to note that it doesn't seem to have made it through the guantlet

[guanfada]

I don't understand why there is error for "tests/gamma/test_agnew_mcgarry.py:119: AssertionError"
this test uses the local gamma, rather then global gamma.
BTW, how was the baseline value obtained? line 151, LOCAL_GAMMA_0_25_BASELINE = 96.9
What new baseline should I use?

[SimonBiggs]

this test uses the local gamma, rather then global gamma.

You're right, that is very odd. Might you be in a position to investigate where the issue may have cropped up?

[guanfada]

this test uses the local gamma, rather then global gamma.

You're right, that is very odd. Might you be in a position to investigate where the issue may have cropped up?

Sure, I will be on it.

[guanfada]

@SimonBiggs @Matthew-Jennings
Today I checked the Delta4 software and confirmed that Delta4 uses global normalization for gamma index calculation by default.
The default global normalization factor is the maximum dose in the treatment plan (within the whole cylindrical phantom of Delta4). It also gives other options to use as the normalization factor.
By default, it uses 10% of this maximum plan dose at the cutoff for gamma calculations, same as PyMedPhys' gamma().
If the measured dose is below it, no gamma will be calculated.
It also sets 500% dose as the upper limit for gamma calculations.

[SimonBiggs]

But I also think diff tools should be configurable to ignore this particular difference as well.

A downside of ignoring it and letting it change from commit to commit, is if a file did not change except for the line endings in some locations that would result in an entirely different hash for that file, resulting in an equivalent file needing to be re-saved within the git tree. I think I'd prefer to have a tool that simply forces the line endings to not have carriage returns. (git can be configured to silently remove all carriage returns within the commit, but leave the carriage returns within the file saved on disk, I believe it is the default when you install git on Windows).

...it's odd to me that GitHub doesn't do the same thing...

[SimonBiggs]

@guanfada I've also found the language of "evaluation" and "reference" ambiguous. Within your comment (#1603 (comment)) you reference TG218. They use the language "planned" for evaluation and "measured" for reference. This use of evaluation and reference is also what Low used in his original paper (except he called it calculated and measured).

I think there would be a benefit and a reduction of ambiguity (resulting in more consistency in the use of the library) if instances of the word evaluation were changed to "planned" and reference was changed to "measured". We would need to still keep the current API for backwards compatibility, however it would be marked deprecated, and when it is used a deprecated warning will be provided to the user.

I think it would also be helpful when referring to these different dose distributions within the docs to make it clear that the planned/evaluation is the distribution being "searched over" and the measured/reference is the one from which the fixed points are being taken (and therefore the final gamma distribution matches that of the reference coords, not the evaluation coords). And also, when the planned/evaluation distribution is monte-carlo derived the noise within the planned distribution will result in the gamma search approach artificially inflating the passing rate.

What is everyone's thoughts?

[sjswerdloff]

But I also think diff tools should be configurable to ignore this particular difference as well.

A downside of ignoring it and letting it change from commit to commit, is if a file did not change except for the line endings in some locations that would result in an entirely different hash for that file, resulting in an equivalent file needing to be re-saved within the git tree. I think I'd prefer to have a tool that simply forces the line endings to not have carriage returns. (git can be configured to silently remove all carriage returns within the commit, but leave the carriage returns within the file saved on disk, I believe it is the default when you install git on Windows).

...it's odd to me that GitHub doesn't do the same thing...

I didn't intend to suggest that these should be ignored, but I also didn't want to start comparing specific diff tools. My preferred diff tool allows one to toggle between identifying 'minor' differences or not, which comes down to things like newline, tab, space, and carriage return. It's not intended for automation, but to ease the cognitive burden on the reviewer when the reviewer deems it appropriate. I am a big fan of lowering cognitive burden in code review (I spend nearly half my time doing code review...)

Given how PyMedPhys is organised, I agree with the idea of enforcing the stripping of Carriage Return as part of pre-commit or commit.

[SimonBiggs]

I am a big fan of lowering cognitive burden in code review

Yeah, that makes a massive amount of sense.

(One of the reasons I really like black)

[guanfada]

@guanfada I've also found the language of "evaluation" and "reference" ambiguous. Within your comment (#1603 (comment)) you reference TG218. They use the language "planned" for evaluation and "measured" for reference. This use of evaluation and reference is also what Low used in his original paper (except he called it calculated and measured).

I think there would be a benefit and a reduction of ambiguity (resulting in more consistency in the use of the library) if instances of the word evaluation were changed to "planned" and reference was changed to "measured". We would need to still keep the current API for backwards compatibility, however it would be marked deprecated, and when it is used a deprecated warning will be provided to the user.

I think it would also be helpful when referring to these different dose distributions within the docs to make it clear that the planned/evaluation is the distribution being "searched over" and the measured/reference is the one from which the fixed points are being taken (and therefore the final gamma distribution matches that of the reference coords, not the evaluation coords). And also, when the planned/evaluation distribution is monte-carlo derived the noise within the planned distribution will result in the gamma search approach artificially inflating the passing rate.

What is everyone's thoughts?

I agree to your comments. @SimonBiggs

[SimonBiggs]

Hi @guanfada,

I've fixed the line-endings issue from prior, so this PR is a little more manageable. Might you be in a position to complete the following task?

@guanfada are you able to track down where the following test is within PyMedPhys and update the baseline value now that the default normalisation has changed:

https://github.com/pymedphys/pymedphys/runs/4636766556?check_suite_focus=true#step:39:246

[guanfada]

@SimonBiggs I will work on it this weekend. Thanks.
~Fada

[SimonBiggs]

Thanks @guanfada,

No stress if you can't! :) Only if it's something you'd enjoy doing. (There are many other enjoyable things one can do on a weekend 🙂)

[guanfada]

global_normalisation has been changed to np.max(dose_evaluation)

[guanfada]

what should I do to complete this merge? Thanks @SimonBiggs

[SimonBiggs]

what should I do to complete this merge? Thanks @SimonBiggs

If you pull the code locally and then run poetry run pymedphys dev tests there will be some tests that this code change ends up breaking. These need to be "got to the bottom of". If they were just baselines, then new baselines need to be made. If it's something else, then that might need more work......

[guanfada]

I just ran it locally.
poetry run pymedphys dev tests

what's the next step? @SimonBiggs

[SimonBiggs]

I just ran it locally. poetry run pymedphys dev tests

what's the next step? @SimonBiggs

Next step is a bit of a problem solving one, one needs to work out why the tests are failing because of this change... and then fix either the tests or the code...

[guanfada]

Do I need to submit a new PR? What commands do I need to do next? I forgot those steps you told me. Thanks.

…

On Apr 3, 2022, at 2:41 AM, Simon Biggs ***@***.***> wrote:  I just ran it locally. poetry run pymedphys dev tests what's the next step? @SimonBiggs Next step is a bit of a problem solving one, one needs to work out why the tests are failing because of this change... and then fix either the tests or the code... — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[SimonBiggs]

Hi @sjswerdloff or @pchlap,

Might either of you be in a position to help here? (Or @Matthew-Jennings if you're available)

[guanfada]

@SimonBiggs
What were the procedures in the past when you updated the code of PyMedPhys?
Thanks！

[pchlap]

Hi @sjswerdloff or @pchlap,
Might either of you be in a position to help here? (Or @Matthew-Jennings if you're available)

Sure @SimonBiggs, I'll give it a try.

@SimonBiggs What were the procedures in the past when you updated the code of PyMedPhys? Thanks！

@guanfada, I'm not particularly familiar with this portion of the code, but perhaps we can figure this out together. As you mentioned above you aren't expecting this test to fail since your change should only affect the local gamma. However, the function modified runs for both local and global gamma. There is a parameter passed to indicate if local_gamma should be computed, but that isn't checked at the point where global_normalisation is set.

The following would make the test pass.... But I am not familiar enough with local vs global gamma to know if this is what we actually want here or not.

        if global_normalisation is None:

            if local_gamma:
                global_normalisation = np.max(dose_reference)
            else:
                global_normalisation = np.max(dose_evaluation)

What do you think?

[Matthew-Jennings]

Hi all, I'm sorry I missed this discussion.

I think there would be a benefit and a reduction of ambiguity (resulting in more consistency in the use of the library) if instances of the word evaluation were changed to "planned" and reference was changed to "measured". We would need to still keep the current API for backwards compatibility, however it would be marked deprecated, and when it is used a deprecated warning will be provided to the user.

I strongly prefer the current nomenclature of "reference/evaluation" over "planned/measured". Not all applications of gamma represent comparisons between planned and measured dose distributions. Planned vs. planned is itself very common now. Moreover, the choice of whether a "planned" or "measured" dose distribution should represent the reference dose distribution is itself application-specific.

I think it would also be helpful when referring to these different dose distributions within the docs to make it clear that the planned/evaluation is the distribution being "searched over" and the measured/reference is the one from which the fixed points are being taken (and therefore the final gamma distribution matches that of the reference coords, not the evaluation coords). And also, when the planned/evaluation distribution is monte-carlo derived the noise within the planned distribution will result in the gamma search approach artificially inflating the passing rate.

But I do agree with these two points, excluding the "planned/measured" nomenclature point.

I'll quickly check which dose distribution Verisoft uses for global normalisation. Maybe FilmQApro too. (I'm now in a non-clinical role and don't recall). If there's inconsistency, it's not clear to me which distribution to choose as the default for normalisation.

EDIT: Perhaps a compromise to address the ambiguity that @SimonBiggs describes is to include an example in the docs (likely @nlambriICH's tute!) that demonstrates a common scenario of comparing planned to measured doses.

[SimonBiggs]

I'll quickly check which dose distribution Verisoft uses for global normalisation. Maybe FilmQApro too. (I'm now in a non-clinical role and don't recall). If there's inconsistency, it's not clear to me which distribution to choose as the default for normalisation.

Let me know how you go on this front. I suspect it will be the decider on whether or not to move forward with this or close this PR.

[Matthew-Jennings]

I'll quickly check which dose distribution Verisoft uses for global normalisation. Maybe FilmQApro too. (I'm now in a non-clinical role and don't recall). If there's inconsistency, it's not clear to me which distribution to choose as the default for normalisation.

Let me know how you go on this front. I suspect it will be the decider on whether or not to move forward with this or close this PR.

Will do. I'll try for Monday.

[guanfada]

@Matthew-Jennings @SimonBiggs
I checked FilmQA pro user manual and its application, but could not find what is used as its global normalization.
Anyway, can we override this default global normalization in the user's implementation? I never tried that.

[SimonBiggs]

Anyway, can we override this default global normalization in the user's implementation? I never tried that.

Yes of course. Essentially just set the parameter global_normalisation=np.max(dose_grid_to_normalise_to)

[SimonBiggs]

I suspect it will be the decider on whether or not to move forward with this or close this PR.

As a note from above (which I had forgotten) @guanfada had already checked the Delta4 manual and it states that it normalises based on the evaluation grid (opposite to what is currently implemented here).

[guanfada]

As a note from above (which I had forgotten) @guanfada had already checked the Delta4 manual and it states that it normalises based on the evaluation grid (opposite to what is currently implemented here).

@SimonBiggs Nevertheless, after my recent investigation, I feel it is best for us to just keep the global_normalisation as the maximum of dose_reference. My reasons are as follows:
(1), using what value as the global normalisation factor is the user's choice, and it definitely can affect the final gamma pass rate.
(2), in Delta4, by default, the global normalisation factors for the composite and its each field are also different. For composite, it is the max dose in the treatment plan (sum of all fields). For each field, it is the max dose in that specific field in the treatment plan. Users have the choice to change the global normalisation factors. For example, they can use the prescription dose, i.e., 2 Gy/fx as the global normalisation factor too.
(3), in FilmQA pro, I don't know what is used as the normalization factor. Maybe it is the prescription dose.
(4), in Eclipse's portal dosimetry, I also don't know what is used as the normalization factor.

(5), this is the main reason that stops me to make changes.
In the 3D gamma demonstration example, below are defined:
max_ref_dose = np.max(dose_reference)
lower_dose_cutoff = gamma_options['lower_percent_dose_cutoff'] / 100 * max_ref_dose
relevant_slice = (
np.max(dose_reference, axis=(1, 2)) >
lower_dose_cutoff)
slice_start = np.max([
np.where(relevant_slice)[0][0],
0])
slice_end = np.min([
np.where(relevant_slice)[0][-1],
len(z_ref)])

And then we show the results on the "reference" slices.
If we change global_normalisation from np.max(dose_reference) to np.max(dose_evaluation), the demonstration results will be very confusing and do not make sense to use the evaluation dose limits as the limits in the reference dose slices.

In all, I would like to withdraw my request to change the default global normalisation factor.
We may add a note in the example, and let the users decide what they like to use.
Let's close this request. What do you think?

[SimonBiggs]

In all, I would like to withdraw my request to change the default global normalisation factor.
We may add a note in the example, and let the users decide what they like to use.
Let's close this request. What do you think?

I'm okay with that 🙂. I think at the end of the day the best path forward might be what @Matthew-Jennings suggested:

Perhaps a compromise to address the ambiguity that @SimonBiggs describes is to include an example in the docs (likely @nlambriICH's tute!) that demonstrates a common scenario of comparing planned to measured doses.

Ideally we would empower users to make an informed sensible choice 🙂, while also providing sensible defaults 🙂.