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

JOSS review item: CI #75

Closed
Matthew-Jennings opened this issue Apr 4, 2024 · 6 comments · Fixed by #86
Closed

JOSS review item: CI #75

Matthew-Jennings opened this issue Apr 4, 2024 · 6 comments · Fixed by #86

Comments

@Matthew-Jennings
Copy link
Contributor

  1. FWIW, my investigations show ~75% test coverage. This isn't too bad, but it might be worth dedicating some spare time to lifting it a little, especially for the files with little-to-no coverage. See mirp_coverage.xlsx for per-file coverage details.

  2. Improvements to temporary test directory handling are in Implement tmp_path #74. Not important for JOSS publication.

  3. Comments related to implementation of GitHub CI can be found here and here. Not important for JOSS publication.

alexzwanenburg added a commit that referenced this issue Apr 4, 2024
Unused code identified in #75.
alexzwanenburg added a commit that referenced this issue Apr 4, 2024
Filter was missing coverage in #75.
@alexzwanenburg
Copy link
Member

I have increased test coverage to 89% by writing additional tests. Some tests remain.

@Matthew-Jennings
Copy link
Contributor Author

Matthew-Jennings commented Apr 7, 2024

I get 84%. See mirp_coverage_24-04-07.xlsx. Not quite 89%! But probably enough 😄

For fair comparison, I'm using pytest-cov with the following command run from the repo's root:

pytest -v --color=yes --cov=mirp --cov-report html --cov-report term --cov-report xml:cov.xml -n=14

Very pleased to find that I was able to run pytest-xdist (the -n=14) with no issue!

@alexzwanenburg
Copy link
Member

Thanks again for the pull request that made pytest-xdist possible. One difference may be that my coverage results included the tests themselves.

@alexzwanenburg alexzwanenburg linked a pull request Apr 11, 2024 that will close this issue
@alexzwanenburg
Copy link
Member

alexzwanenburg commented Apr 11, 2024

To expose (and test) SUV conversion we need to do the following:

  • Add piping for passing arguments from StandardWorkflow.standard_image_processing to ImageDicomFilePT.load_data.
  • Expose the following parameters by adding them to ImagePostProcessingClass:
    • suv_type with default body_weight.
    • decay_correction_event with default administration. There is no practical reason why decay correction should be configurable.
  • Add documentation for new parameters.
  • Add parameters to xml.
  • Integrate SUVScalingObj into ImageDicomFilePT as private methods, since the required metadata for SUV conversion is not present in different file formats.
    • After consideration: split SUVScalingObj into parts related to decay correction, and parts related to SUV conversion.
    • Decay correction should take place as part of load_data.
    • SUV conversion (for now) takes place as part of load_data. In the future data conversion may move into the standard workflow to make it more explicit. This would require adding required data (weight, height, biological sex, voxel units) to PETImage objects.
  • Add unit tests for parameters and parameter combinations.

@alexzwanenburg
Copy link
Member

I made some progress in coverage with version 2.2.1. I am leaving this issue open as I need to create new test data to test remaining parts of the code.

@Matthew-Jennings
Copy link
Contributor Author

No worries, @alexzwanenburg. At 86% coverage, I'm happy to consider this item "closed" from a JOSS review perspective.

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 a pull request may close this issue.

2 participants