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

Brainprint PyTests #19

Merged
merged 12 commits into from
Mar 25, 2024
Merged

Conversation

engrosamaali91
Copy link
Contributor

Method to run brainprint test cases using pytest:

  • Create python virtual environment
  • pip install pytest
  • cd /test_cases
  • run command pytest filename.py

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #19 (cd606cd) into main (1b89f5a) will not change coverage.
Report is 9 commits behind head on main.
The diff coverage is n/a.

❗ Current head cd606cd differs from pull request most recent head f8a5e9d. Consider uploading reports for the commit f8a5e9d to get more accurate results

@@           Coverage Diff           @@
##             main      #19   +/-   ##
=======================================
  Coverage   26.62%   26.62%           
=======================================
  Files           8        8           
  Lines         353      353           
  Branches       43       43           
=======================================
  Hits           94       94           
  Misses        256      256           
  Partials        3        3           
Flag Coverage Δ
unittests 26.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
brainprint/brainprint.py 20.58% <ø> (ø)
brainprint/surfaces.py 20.37% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@m-reuter
Copy link
Member

Thanks, can you:

  • rebase to current main so that we don't see the changes that were already merged.
  • fix the black error.
  • move the test to the brainprint/utils/test directory so that they are run with the other pytest workflows.

@engrosamaali91
Copy link
Contributor Author

In this commit cd606cd only errors were resolved. The tests have not been moved to brainprint/utils/test yet.

@engrosamaali91
Copy link
Contributor Author

Apparently f8a5e9d automated tests are not possible on github as it requires FREESURFER export and subject directory as well.
However the pytests for brainprint can be done locally.

@m-reuter
Copy link
Member

Yes, but are there any tests that can be done without FreeSurfer to be installed? We would not want to install all of FreeSurfer as that is too large and takes too long. But we can replace several of the freesurfer binaries with our own python scripts, for example mri_binarize just combines labels, and there is also code for marching-cube (to extract surfaces), that could increase the number of tests that do not need FreeSurfer.

@kdiers
Copy link
Collaborator

kdiers commented Oct 19, 2023

I think @m-reuter's comment touches the more general question how independent of FreeSurfer this software can be already. To my understanding, the only FreeSurfer binaries that we use are mri_binarize, mri_pretess, mri_mc, and mris_convert (all of them called in surfaces.py). Not exactly sure about mri_pretess (and if it's strictly needed), but I think all others could be replaced by python code. We might not be able to exactly reproduce FreeSurfer-based results, but I guess results obtained without FreeSurfer-based code would be valid as well. In that sense, it might be rather an issue of compatibility between versions of this software. Moving away from FreeSurfer would require some development and validation, but if we want to do this, then it would also allow to run all tests remotely - not just some.

@kdiers kdiers changed the base branch from main to add-tests March 25, 2024 16:29
@kdiers
Copy link
Collaborator

kdiers commented Mar 25, 2024

Redirecting the pull request from the previous target branch (main) to a new branch (add-tests), because the pull request still needs to be modified, but pushing to the external, forked repository is no longer desired. Will eventually merge the pull request into the new target branch, do necessary modifications, and after that create another PR back into the main branch.

@m-reuter m-reuter merged commit ba14829 into Deep-MI:add-tests Mar 25, 2024
15 of 27 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.

3 participants