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

Changed the polyfit to compute function #2223

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Changed the polyfit to compute function #2223

merged 1 commit into from
Jul 15, 2024

Conversation

ashmeigh
Copy link
Collaborator

Issue

Closes #2051

Description

This pull request refactors the polyfit_correlation module to transition from the partial function style to the compute_function approach. This change improves code clarity, maintainability, and performance.

Changes
Refactored find_center to use the new compute_function method.
Introduced compute_correlation_error to encapsulate core logic for parallel execution.
Allocated shared memory for arrays to enhance performance.
Updated code to handle shared memory arrays and ensure compatibility.

Testing

Verifiy the refactored find_center function by running unit tests.
Ensure that compute_correlation_error correctly computes correlation errors in parallel.
Test shared memory allocation and access in a multiprocessing context.

Acceptance Criteria

Ensure all unit tests pass.
Verify that the find_center function performs as expected with different image inputs.
Check that the compute_function handles shared memory correctly without errors.

@ashmeigh ashmeigh requested a review from samtygier-stfc June 11, 2024 14:41
@samtygier-stfc
Copy link
Collaborator

I get the hang when I run locally with
python -m pytest -vs mantidimaging/core/rotation/test/polyfit_correlation_test.py -k test_find_center

Also some error

mantidimaging/core/rotation/test/polyfit_correlation_test.py::PolyfitCorrelationTest::test_find_center Exception ignored in: <function SharedArray.__del__ at 0x7760ac343910>
Traceback (most recent call last):
  File "/home/sam/git/mantidimaging/mantidimaging/core/parallel/utility.py", line 155, in __del__
    if self.has_shared_memory:
  File "/home/sam/git/mantidimaging/mantidimaging/core/parallel/utility.py", line 166, in has_shared_memory
    return self._shared_memory is not None
AttributeError: 'SharedArray' object has no attribute '_shared_memory'
Exception ignored in: <function SharedArray.__del__ at 0x711fd2343910>

@MikeSullivan7
Copy link
Collaborator

I get the hang when I run locally with python -m pytest -vs mantidimaging/core/rotation/test/polyfit_correlation_test.py -k test_find_center

Also some error

mantidimaging/core/rotation/test/polyfit_correlation_test.py::PolyfitCorrelationTest::test_find_center Exception ignored in: <function SharedArray.__del__ at 0x7760ac343910>
Traceback (most recent call last):
  File "/home/sam/git/mantidimaging/mantidimaging/core/parallel/utility.py", line 155, in __del__
    if self.has_shared_memory:
  File "/home/sam/git/mantidimaging/mantidimaging/core/parallel/utility.py", line 166, in has_shared_memory
    return self._shared_memory is not None
AttributeError: 'SharedArray' object has no attribute '_shared_memory'
Exception ignored in: <function SharedArray.__del__ at 0x711fd2343910>

This seems to be a Linux specific issue, as I dont get this on Windows. Makes sense with it being related to the Shared Memory.

@samtygier-stfc
Copy link
Collaborator

I think the problem here is that SharedArray objects are being passed to the compute function as params where they need to be in using arrays. params should only contain simple types.

This is needed because SharedArrays need to be converted to SharedArrayProxys before being passed to the other processes (see _check_shared_mem_and_get_data). If this does not happen then the other process thinks it owns the SharedArray and therefore tries to deleted it when it goes out of scope.

@coveralls
Copy link

coveralls commented Jul 8, 2024

Coverage Status

coverage: 72.992% (-0.06%) from 73.052%
when pulling a5e7ada on compute_poly
into 967daed on main.

@samtygier-stfc
Copy link
Collaborator

I've rebased and squashed the commits to reduce the churn.

@samtygier-stfc samtygier-stfc added this pull request to the merge queue Jul 15, 2024
Merged via the queue into main with commit f3c138f Jul 15, 2024
8 checks passed
@samtygier-stfc samtygier-stfc deleted the compute_poly branch July 15, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Move operations to compute_function style
4 participants