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

Find centre of rotation (tentative 2) #30

Merged
merged 22 commits into from
Nov 26, 2024
Merged

Find centre of rotation (tentative 2) #30

merged 22 commits into from
Nov 26, 2024

Conversation

lauraporta
Copy link
Member

Tentative 2 with squashed commits

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
See #12

What does this PR do?

Creates methods and pipeline to find the centre of rotation in the incremental dataset.

⚠️ It does not fix the wobbling issues. The pipeline works well with simulated data but worsens the real datasets.

Pipeline

Find the centre

Find the centre of rotation using the incremental rotations datasets, in which the sample was rotated at discrete angles with 10deg intervals. Centre of rotation is found by fitting an ellipse to the brightest ROI location at each of the 10deg configurations.
New methods include:

  • find_center_of_rotation
  • get_coords_of_largest_blob
  • plot_blob_detection
  • fit_ellipse_to_points
  • plot_ellipse_fit_and_centers

ellipse_fit

max_projection_with_center

Use the new centre

In order to use the new centre of rotation, the derotate_an_image_array_line_by_line needed to be totally rewritten. I am discarding the usage of scipy.ndimage.rotate and using classic linear algebra to rotate the matrices. With this new implementation the derotation is so much faster! 🚀

How has this PR been tested?

The performance of the de-rotation when the centre of rotation is shifted has been tested with simulated data generated by the class Rotator.

I created a stepwise array of angles to imitate the incremental rotation dataset, and a sinusoidal array as the main experimental session dataset.
rotation_angles

I then rotated a sample image that has two blobs that imitate two ROIs (one brighter than the other).
test_image

Ellipse fit works well with a shifted centre:
ellipse_fit

And derotation works smothly. This is the proof that in principle my derotation with shifted centres works.

I added tests to cover this new functionality and the pipeline.

Is this a breaking change?

No

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

commit 16734d0
Author: lauraporta <[email protected]>
Date:   Fri Oct 25 13:59:25 2024 +0100

    Add docstrings and description on the very long test integration module

commit 89f8a77
Author: lauraporta <[email protected]>
Date:   Fri Oct 25 11:55:19 2024 +0100

    Small refactor conftests

commit eb60b72
Author: lauraporta <[email protected]>
Date:   Thu Oct 24 18:36:25 2024 +0100

    Move out assertions handling in a separate function, handle paths differently and reorganize part of the fixtures

commit 758a498
Author: lauraporta <[email protected]>
Date:   Thu Oct 24 17:31:21 2024 +0100

    Add spaces to improve table formatting

commit 141daa4
Author: lauraporta <[email protected]>
Date:   Thu Oct 24 17:23:26 2024 +0100

    Add readme to describe the way regression tests work

commit 02fbadd
Author: lauraporta <[email protected]>
Date:   Thu Oct 24 17:02:45 2024 +0100

    Add more comments, docstrings and typing in regression tests files to regenerate target images

commit 5a83168
Author: lauraporta <[email protected]>
Date:   Thu Oct 24 16:07:13 2024 +0100

    Remove repeated functions

commit fc2d9a7
Author: lauraporta <[email protected]>
Date:   Thu Oct 24 15:34:23 2024 +0100

    Move in separate files the logic to recreate the images for regression tests

commit 5137eb2
Author: lauraporta <[email protected]>
Date:   Thu Oct 17 21:21:07 2024 +0100

    Apply suggestions from PR review

commit cf3998d
Author: lauraporta <[email protected]>
Date:   Mon Oct 14 14:16:54 2024 +0100

    Simplify filter

commit 3f631bf
Author: lauraporta <[email protected]>
Date:   Fri Oct 4 11:32:45 2024 +0100

    Move the plots of the integration script into the integration test

commit aa37d4e
Author: lauraporta <[email protected]>
Date:   Fri Oct 4 11:16:48 2024 +0100

    Fix mypy error messages

commit ff55b72
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 18:30:46 2024 +0100

    WIP docs

commit 0737e65
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 16:50:59 2024 +0100

    Update workflows and add dependabot yml

commit 81a72c8
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 16:35:39 2024 +0100

    Increase the tolerance and improve error message

commit e45ff2c
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 16:16:34 2024 +0100

    Let's accept little differences in the derotation

commit a941f41
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 16:00:01 2024 +0100

    Also do't need the folder

commit 1f8ded8
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 15:57:42 2024 +0100

    Don't need to save plots in tests

commit ef186f9
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 15:52:45 2024 +0100

    Add integration tests on the integration of the two pipelines to find the center of rotation

commit 10ec8e8
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 15:16:13 2024 +0100

    Move logic to get the largest blob in a separate method

commit a211832
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 14:33:17 2024 +0100

    Move the plots out of the main function in integration script

commit 7a00c37
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 14:13:09 2024 +0100

    Fix pytest error (missing required property)

commit d69ec18
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 14:12:43 2024 +0100

    Uncomment part of plotting hooks

commit dddbaf3
Author: lauraporta <[email protected]>
Date:   Thu Oct 3 14:12:30 2024 +0100

    Reorganize examples

commit 48c8b54
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 17:10:45 2024 +0100

    Add another debugging plot

commit 3d5c1ce
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 17:10:14 2024 +0100

    Use the script to test with a different center of rotation, works when checking for correct order of x-y coordinates

commit 52be41c
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:51:07 2024 +0100

    Use blank pixel

commit 8963f5f
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:43:58 2024 +0100

    Comment one hook

commit a4dc480
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:37:18 2024 +0100

    WIP: script to test the integration of the two pipelines to find the center of rotation and then derotate

commit d783005
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:35:33 2024 +0100

    WIP: finding the center of rotation via ellipse fitting

commit fde0592
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:31:33 2024 +0100

    Fix angle indices bug in Rotator

commit ca6cc03
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:28:47 2024 +0100

    Plotting bug fixed

commit 0dd3c6d
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:28:06 2024 +0100

    Change variable names in plotting hooks

commit bfa4443
Author: lauraporta <[email protected]>
Date:   Mon Sep 30 15:25:35 2024 +0100

    Remove hooks from pipeline

commit 2779819
Author: lauraporta <[email protected]>
Date:   Fri Sep 27 14:37:27 2024 +0100

    Clean up and refactor the main pipeline around the center of rotation feature - also make separate module for hooks

commit edee980
Author: lauraporta <[email protected]>
Date:   Fri Sep 27 14:10:04 2024 +0100

    Add wrongly deleted hook

commit c4f9d14
Author: lauraporta <[email protected]>
Date:   Fri Sep 27 14:09:21 2024 +0100

    Add test for derotation with different centre

commit 0721e5d
Author: lauraporta <[email protected]>
Date:   Fri Sep 27 13:53:58 2024 +0100

    Add new generated derotated squares: faster derotation by line leads to worse performance...

commit df141be
Author: lauraporta <[email protected]>
Date:   Fri Sep 27 13:19:23 2024 +0100

    Update tests with new derotation version

commit 765c4de
Author: lauraporta <[email protected]>
Date:   Fri Sep 27 13:14:36 2024 +0100

    Update tests rotator with different center

commit c0423a0
Author: lauraporta <[email protected]>
Date:   Fri Sep 27 11:49:39 2024 +0100

    Remove linear interpolation and recompute test images - now it's a square of different gray lines

commit 1412348
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 16:21:46 2024 +0100

    Resolve naming (in many cases with "rotation" I meant derotation)

commit 860a2fa
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 16:08:21 2024 +0100

    WIP: changing the main derotation by line algorithm

commit e5cac06
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 15:50:05 2024 +0100

    Update basic rotator to include different center of rotation

commit 35a0b7f
Merge: a0c6017 c825a82
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 15:06:45 2024 +0100

    Merge commit 'c825a828116dd382d8e9f8e1695c46adcdb94ecc' into feature/finding-centre-of-rotation

commit c825a82
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 14:59:10 2024 +0100

    Delete duplicate file

commit c3cf60e
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 14:42:53 2024 +0100

    Add tests

commit 55b080e
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 14:41:05 2024 +0100

    Fix Rotator and join it with the derotator in an example

commit 4832672
Author: lauraporta <[email protected]>
Date:   Thu Sep 26 14:39:37 2024 +0100

    Make function name consistent

commit c913a06
Author: lauraporta <[email protected]>
Date:   Wed Sep 25 17:40:25 2024 +0100

    Add basic tests

commit dfe659a
Author: lauraporta <[email protected]>
Date:   Wed Sep 25 17:15:19 2024 +0100

    Add docstrings

commit 29e9d27
Author: lauraporta <[email protected]>
Date:   Wed Sep 25 17:11:07 2024 +0100

    Add basic rotator and a simple usage of it

commit a0c6017
Author: lauraporta <[email protected]>
Date:   Wed Sep 25 16:28:37 2024 +0100

    WIP 🏗️: draft pipeline for adapting center of rotation

commit 6000609
Author: lauraporta <[email protected]>
Date:   Wed Sep 25 16:24:55 2024 +0100

    Improve debug plots

commit 9bf17eb
Author: lauraporta <[email protected]>
Date:   Wed Sep 25 16:24:26 2024 +0100

    Add plotting hooks and further params to main derotate by line function

commit aae7c67
Author: lauraporta <[email protected]>
Date:   Mon Sep 23 16:29:24 2024 +0100

    Fixes debugging plots and logging usage

commit 174fd9e
Author: lauraporta <[email protected]>
Date:   Thu Sep 19 16:48:00 2024 +0100

    Include usage of center of rotation in main pipeline

commit f6adedf
Author: lauraporta <[email protected]>
Date:   Thu Sep 19 16:35:41 2024 +0100

    Add working methods to find center of rotation 🎉

commit b76a30a
Author: lauraporta <[email protected]>
Date:   Mon Sep 16 11:36:50 2024 +0100

    Reorganize methods order

commit 168100a
Author: lauraporta <[email protected]>
Date:   Mon Sep 16 11:26:07 2024 +0100

    Add empty method
@lauraporta
Copy link
Member Author

From previous PR:

Improve tests, suggestions from @JoeZiminski in #25 :

  • explain better the strategy of regression tests. Comments 1, 2
  • path of the pre-computed images. Comments 3
  • naming of the images. Comments 4
  • expand tests on Rotator constructor. Comments 5
  • move fixtures to conftest.py. Comments 6
  • more notes on why did I made certain choices. Comments 7

@lauraporta lauraporta mentioned this pull request Nov 15, 2024
7 tasks
@lauraporta lauraporta changed the title Squashed commit of the following: Find centre of rotation (tentative 2) Nov 15, 2024
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 55.17241% with 65 lines in your changes missing coverage. Please review.

Project coverage is 43.89%. Comparing base (dd0bfab) to head (a1ece6a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...tation/analysis/incremental_derotation_pipeline.py 51.48% 49 Missing ⚠️
derotation/analysis/full_derotation_pipeline.py 12.50% 14 Missing ⚠️
derotation/derotate_by_line.py 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
- Coverage   48.54%   43.89%   -4.65%     
==========================================
  Files           5        6       +1     
  Lines         377      631     +254     
==========================================
+ Hits          183      277      +94     
- Misses        194      354     +160     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lauraporta lauraporta marked this pull request as ready for review November 15, 2024 15:48
Copy link
Member

@JoeZiminski JoeZiminski left a comment

Choose a reason for hiding this comment

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

Hey @lauraporta this looks great, very easy to follow and super clean! The tests and plots are really nice, it's super useful for understanding and very convincing that the algorithm is working well. I don't really have any major feedback, just a few things that came to mind. The only thing is the pattern (assuming I understand correctly) to re-generate the regression test data by running the test script as main vs. just testing with pytest. It seems it could be easy to accidentally run the script and regenerate the data, which would then nullify all tests. I don't think this is that major for now but might be something to think about in future.

I am super curious as to why incorporating the centre of rotation is making the real-world case worse 🤔 but I guess outside the scope of this PR!

@lauraporta lauraporta merged commit c88975e into main Nov 26, 2024
24 of 25 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.

2 participants