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

350 utils #351

Merged
merged 28 commits into from
Jul 18, 2023
Merged

350 utils #351

merged 28 commits into from
Jul 18, 2023

Conversation

sophie22
Copy link
Collaborator

@sophie22 sophie22 commented Jul 6, 2023

Summary

in line with Python best practice, remove all commands possible from the init.py into a new main.py script, make required changes to entry point and subsequently update the READme with the relevant commands
EDIT: upon further reading of documentation, looks like there is no real benefit to having the main function in a separate script from init.

combine utility functions from init, tools and shapes.py into a new utils.py, make required changes to all task scripts that use utility functions and their applicable test scripts

resolves #347

Outstanding TODO:

  • update original test_tools and test_hazenlib scripts and correctly group unit tests in line with new utils.py and main.py

@sophie22 sophie22 requested a review from tomaroberts July 6, 2023 11:39
@github-actions
Copy link

github-actions bot commented Jul 11, 2023

Coverage

Coverage Report
FileStmtsMissCoverMissing
hazenlib
   HazenTask.py26388%32–34
   __init__.py531964%109–119, 150, 162, 164–166, 168–174, 179–181, 189
   exceptions.py21481%17–21
   relaxometry.py3119071%248–266, 640, 699–701, 755, 803–825, 843–858, 1186–1189, 1198–1201, 1213–1226, 1229–1234, 1245–1274
   utils.py1894377%61, 65, 75, 80, 117, 124–129, 140, 143–150, 170–172, 190–192, 211–213, 222, 227, 233, 284, 287, 295–300, 303, 346, 355, 371
hazenlib/tasks
   acr_geometric_accuracy.py1465662%38–75, 179–195, 209–233
   acr_ghosting.py1174363%33–55, 93–95, 125–127, 163–196
   acr_slice_position.py1555465%53–76, 154, 215–260
   acr_slice_thickness.py1546458%40–62, 188–243
   acr_snr.py1396057%34–75, 100, 169–179, 212–225, 258–271
   acr_spatial_resolution.py2447470%66–90, 169, 212, 225–234, 323–378
   acr_uniformity.py903363%34–56, 123–140
   ghosting.py1525365%18–35, 50, 112–113, 117, 127–128, 154–156, 173–175, 221–259
   relaxometry.py770%1–11
   slice_position.py1172380%28, 37–38, 49, 103–104, 130, 210, 217–234
   slice_width.py3605385%34–37, 41, 109, 168–188, 453, 458–459, 465, 470, 532–533, 782–823
   snr.py1646759%48, 65–70, 164–182, 197–206, 224–234, 261–271, 276–286, 317–330, 335–343, 372–385
   snr_map.py104199%291
   spatial_resolution.py2484582%36–39, 43, 64, 149, 208, 334–370
   uniformity.py792075%42–45, 51, 93–94, 101, 135–149
TOTAL289281272% 

Tests Skipped Failures Errors Time
219 0 💤 0 ❌ 0 🔥 3m 12s ⏱️

@sophie22
Copy link
Collaborator Author

https://realpython.com/python-main-function/
main function is not supposed to have a return value, but could call other functions that do --> will have to update the test_hazenlib unit tests that rely on evaluating the stdout to check output is as expected

@tomaroberts tomaroberts self-assigned this Jul 14, 2023
Copy link
Collaborator

@tomaroberts tomaroberts left a comment

Choose a reason for hiding this comment

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

Changes look great.

Just need to resolve the GitHub Actions issue where the CLI tests fail.

@tomaroberts tomaroberts mentioned this pull request Jul 17, 2023
@sophie22
Copy link
Collaborator Author

sophie22 commented Jul 18, 2023

as discussed last week, GitHub tests fail either the CLI test when tasks complete with exit code 1 due to a string being returned from the main function, or the development tests fail: 2 tests within test_utils rely on having the string returned by main to be compared against expected values.

So to resolve these, I plan to change the main function so it doesn't return anything and printing of the result is handled before the end of main and then modifying the test_hazenlib functions to parse sys.stdout to capture the result printed.
https://pytest.org/en/7.4.x/how-to/capture-stdout-stderr.html

@tomaroberts let me know if this sounds sensible and I'll get on it

@tomaroberts
Copy link
Collaborator

As discussed on Teams, give it a go :)

@sophie22
Copy link
Collaborator Author

made the agreed modifications such that hazenlib.main() does not return anything (completes with exit code 0)
and modified the tests for the SNR and relaxometry tasks such that they do not rely on hazenlib.main() which now doesn't return anything, but instead call their respective run() function with equivalent arguments as passed to CLI parser.

@sophie22
Copy link
Collaborator Author

in the future, I recommend to separate hazenlib.main() into the following sub-functions:

  • parse_CLI_args: purely responsible for presenting a useful help and parsing the passed arguments
    • it allows for extendability by adding new tasks or new optional arguments to existing tasks easily
  • prepare: all sorts of checks on the inputs provided: includes checking that an available task was requested with applicable optional arguments, and that suitable DICOMs exist within the folder specified. Prepare dicom files to be passed to a task and create report output folder, if applicable.
  • run_task: make an instance of the selected task class and call its run() function with arguments as identified above, this is what would return the result dictionary that is printed within the main() function, which returns nothing
    • this is what could be tested in the unit tests that task with known input returns calculated values asa expected

tests/test_hazenlib.py Outdated Show resolved Hide resolved
tests/test_hazenlib.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tomaroberts tomaroberts left a comment

Choose a reason for hiding this comment

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

Great! :)

Couple of lines to remove, unless you think useful to keep.

Go ahead and merge main into this branch and resolve any conflicts.

@tomaroberts tomaroberts merged commit 7d9b5b1 into main Jul 18, 2023
@tomaroberts tomaroberts mentioned this pull request Jul 18, 2023
@tomaroberts tomaroberts deleted the 350_utils branch August 23, 2023 08:00
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.

Improve/fix usage instructions - separate init.py and main.py
2 participants