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

#293 fix to calculate_d_cc_x #294

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

dlbarbee
Copy link
Contributor

@dlbarbee dlbarbee commented Nov 7, 2024

added struct_hash, dose_hash, and label grouping to the calculation in order to differentiate situations where the dvh is a df with multiple repeating labels. previously was repeating the same value for each label across multiple rows with different struct_hash and dose_hash.

Copy link
Contributor

@pchlap pchlap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @dlbarbee, a really useful fix.

There is just one problem that I see with this solution which we should resolve. The use of struct_hash and dose_hash is pydicer specific, so other uses of platipy where pydicer isn't used will have an issue here. This can be seen in the GitHub PR pipelines failing.

My suggestion is to add in a parameter named something like index_columns. This can default to ["label"] which will then be used for grouping and will result in the same behavior as before. However we can then set this to ["struct_hash", "dose_hash", "label"] from pydicers call to this function which will then do the grouping as in your implementation here. Other users could even set this to other columns names if they are passing through a DataFrame in some other form.

I'm happy to make this change. I'll try to push it through directly to this branch and update this PR here but there is a chance I might need you to give me write access to this branch. Ill let you know.

@pchlap
Copy link
Contributor

pchlap commented Nov 10, 2024

@dlbarbee I've made that change. This keeps the behaviour the same for existing users. For pydicer we'll need to ammend the line:

https://github.com/AustralianCancerDataNetwork/pydicer/blob/17d56f610f5ec8678c6b3a9588b2045d02abe2f2/pydicer/analyse/data.py#L289

changing it to:

df_dcc = calculate_d_cc_x(df_dvh, d_cc_point, index_cols=["struct_hash", "dose_hash", "label"])

I'll wait until we merge this through to make that change in pydicer. Let me know what you think and if that fixes things from your perspective. Thanks!

@dlbarbee
Copy link
Contributor Author

That's a much more elegant solutiton that keeps it non-specific! Thanks so much for getting to this so quickly.

I'll make the changes on my end and check it out.

@pchlap pchlap merged commit 86f856a into pyplati:master Nov 19, 2024
4 checks passed
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