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

Summary dictionary for Winston-Lutz in new format #348

Merged
merged 5 commits into from
Mar 31, 2021

Conversation

crcrewso
Copy link
Contributor

This is my second attempt at a format for what was first presented in PR #316

The format of the dictionary would be the same.

@coveralls
Copy link

coveralls commented Mar 16, 2021

Coverage Status

Coverage increased (+0.1%) to 82.937% when pulling 16b30fe on crcrewso:json_summarizer into 0232234 on jrkerns:master.

@jrkerns
Copy link
Owner

jrkerns commented Mar 17, 2021

Hey Cody, thanks for the PR. I will provide some comments in a review. @randlet would a results dictionary be helpful here for QAT?

pylinac/winston_lutz.py Outdated Show resolved Hide resolved
pylinac/winston_lutz.py Outdated Show resolved Hide resolved
pylinac/winston_lutz.py Outdated Show resolved Hide resolved
pylinac/winston_lutz.py Outdated Show resolved Hide resolved
tests_basic/test_winstonlutz.py Outdated Show resolved Hide resolved
@crcrewso
Copy link
Contributor Author

crcrewso commented Mar 17, 2021

@jrkerns These are great notes, which obviously involve some deep philosophical changes to my code. At the time of writing this PR I had not understood the testing infrastructure. Now that I do I think I can write something that accomplishes the same task and depends on unit testing for maintenance rather than the current ideology.

@jrkerns @randlet I wrote this idea specifically so that systems like QATrack could have a universal access method that would be independent of internal function calls to reduce fragile code within QAtrack tests.

@randlet
Copy link
Contributor

randlet commented Mar 17, 2021

A results dictionary would be very helpful, especially if it can be standardized across the different analysis modules/classes.

I think the main thing to shoot for is to have a stable API for accessing the results of analysis, which is what this PR is aiming to do. This would make updating pylinac within QATrack+ a simpler affair. The recent changes to mtf & rois meant that everyone has to update their QATrack+ pylinac tests to account for the new pylinac API. If we had a stable interface for accessing those results then pylinac can feel free to change things internally, but people who only care about the end results don't need to do modify their code after updating versions.

@jrkerns
Copy link
Owner

jrkerns commented Mar 17, 2021

A results dictionary would be very helpful, especially if it can be standardized across the different analysis modules/classes.

I think the main thing to shoot for is to have a stable API for accessing the results of analysis, which is what this PR is aiming to do. This would make updating pylinac within QATrack+ a simpler affair. The recent changes to mtf & rois meant that everyone has to update their QATrack+ pylinac tests to account for the new pylinac API. If we had a stable interface for accessing those results then pylinac can feel free to change things internally, but people who only care about the end results don't need to do modify their code after updating versions.

You really need to get on those repo owners. They'll change the floor beneath your feet.

@jrkerns
Copy link
Owner

jrkerns commented Mar 17, 2021

@jrkerns These are great notes, which obviously involve some deep philosophical changes to my code. At the time of writing this PR I had not understood the testing infrastructure. Now that I do I think I can write something that accomplishes the same task and depends on unit testing for maintenance rather than the current ideology.

@jrkerns @randlet I wrote this idea specifically so that systems like QATrack could have a universal access method that would be independent of internal function calls to reduce fragile code within QAtrack tests.

I resisted this idea initially but having seen the work required to keep pylinac up to date, I can see the benefit now. The good news is this is a pretty incremental change. @crcrewso IMHO the easiest thing would be to revert this commit and just add a results_data() method and then a unit test for it. I can clean it up too later if need be as I'll need to replicate this across modules.

@crcrewso
Copy link
Contributor Author

@jrkerns These are great notes, which obviously involve some deep philosophical changes to my code. At the time of writing this PR I had not understood the testing infrastructure. Now that I do I think I can write something that accomplishes the same task and depends on unit testing for maintenance rather than the current ideology.
@jrkerns @randlet I wrote this idea specifically so that systems like QATrack could have a universal access method that would be independent of internal function calls to reduce fragile code within QAtrack tests.

I resisted this idea initially but having seen the work required to keep pylinac up to date, I can see the benefit now. The good news is this is a pretty incremental change. @crcrewso IMHO the easiest thing would be to revert this commit and just add a results_data() method and then a unit test for it. I can clean it up too later if need be as I'll need to replicate this across modules.

@jrkerns I can go forward with that, (I am assuming we're going ahead with the results_data() method contains all of the logic and no modifications are made anywhere else)

Once you're comfortable with a specific nomenclature and format, then I'll be happy to help replicate it over other methods, as we implement PyLinac across other QAtrack tests.

@jrkerns jrkerns merged commit c485f9a into jrkerns:master Mar 31, 2021
@crcrewso crcrewso deleted the json_summarizer branch March 31, 2021 20:43
jrkerns added a commit that referenced this pull request Feb 29, 2024
RAM-2605 Add quasar and jaw orthogonality contribs. Refactor sized locator.

Approved-by: Randy Taylor
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.

None yet

4 participants