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

Trying to accelerate PET algebra tests for larger data #1027

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evgueni-ovtchinnikov
Copy link
Contributor

trying to fix #1013 for larger data

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

looks good. I didn't check current run-time.

Where does tests/data.hs sit? Is it checked in ? (I didn't check)

self.image1 = data.get_uniform_copy(0)
self.image2 = data.get_uniform_copy(0)
return
path = os.path.join(examples_data_path('PET'),
Copy link
Member

Choose a reason for hiding this comment

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

not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks good. I didn't check current run-time.

Where does tests/data.hs sit? Is it checked in ? (I didn't check)

testing time is now 14 sec

data.hs and data.s are created, if not present, in SIRF/tests by the first test - do not know how to delete them after all tests finished (CMake can, I presume?)

not used?

sorry, do not get what you refer to

@paskino
Copy link
Contributor

paskino commented Dec 15, 2021

I think setUp should just rebin the data as you propose:

        self.set_storage_scheme()
        sirf_path = os.environ.get('SIRF_PATH')
        
        path = os.path.join(examples_data_path('PET'),
                            'mMR', 'mMR_template_span11_small.hs')
        
        template = pet.AcquisitionData(path)
        # print(template.dimensions())
        data = template.get_uniform_copy(0)
        data = data.rebin(3, num_views_to_combine=6, num_tang_poss_to_trim=200)
        # print('rebinned to ', data.dimensions())
        self.image1 = data
        self.image2 = data.get_uniform_copy(0)

Also the file data.hs that you create the first time you run the unit test (actually setUp) should be deleted at the end of the test, during tearDown, making the trick not really viable.

Copy link
Contributor

@paskino paskino left a comment

Choose a reason for hiding this comment

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

See my comment #1027 (comment)

@evgueni-ovtchinnikov
Copy link
Contributor Author

I think setUp should just rebin the data

rebinning in each test takes twice as long (30 sec vs 14 sec)

admittedly my fix is quite ugly, but I do not understand how unittest works, so cannot suggest anything else at the moment

@paskino
Copy link
Contributor

paskino commented Dec 15, 2021

Each unit tests will be preceded by a call to setUp and call to tearDown. The idea here is to create whatever temporary thing required for the test and clear it away even if the test fails.

Your fix is functional, but does go against the whole idea of setUp and tearDown. Why not add small PET AcquisitionData and ImageData files in the repository?

@paskino
Copy link
Contributor

paskino commented Jan 17, 2022

sirf_path = os.environ.get('SIRF_PATH')
data_path = os.path.join(sirf_path, 'tests', 'data.hs')

The file created during the first unittest will end up in ${SIRF_PATH}/tests, which is in the source directory. Does this mean that git will not like it when running a second time?

If so, I believe we should delete the data.hs file once done with the tests. I'm not so sure if there is a single test class created to keep the count of how many tests have been run.

A possible solution is to be pragmatic and think that this file will be used by the PET Python algebra only and we can create it in the build directory so that we reduce the pollution of the source and install directories to a minimum. I'd be in favour of this solution.

@KrisThielemans KrisThielemans added this to the v3.3 milestone Mar 14, 2022
@KrisThielemans KrisThielemans modified the milestones: v3.6, v4.0 Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test PET__PYTHON_ALGEBRA is very slow
3 participants