search

jrkerns / pylinac

An image analysis library for medical physics
https://pylinac.readthedocs.io/en/latest/
MIT License
153 stars 98 forks source link

Summary dictionary for Winston-Lutz in new format #348

Closed crcrewso closed 3 years ago

crcrewso commented 3 years ago

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 commented 3 years ago

Coverage Status

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

jrkerns commented 3 years ago

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

crcrewso commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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.