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

auto generate sampling angles #163

Merged
merged 65 commits into from
Jun 3, 2024
Merged

Conversation

sroet
Copy link
Collaborator

@sroet sroet commented Apr 30, 2024

Will close #92 by auto generating theta and phi with healpix and sampling psi linearly. It will use the input angle as a maximum angle difference

Todo:

  • remove pre-generated angles
  • add angle autogeneration code
  • add new dependencies
  • make sure install still works
  • Update tests (except angle tests) to new type of code
  • update entry points to deal with new angle creation code
  • add angle generation tests
  • add any other tests to cover touched code (except entry_points)
  • test all added functions with mypy
  • test all touched code with ruff (decided to leave this for another PR)
  • format all touched files with black (decided to leave this for another PR)

Other fixes:

The test of bad angles in the angle file was missing newlines, so it did raise the expected type ValueError, just the wrong ValueError

The coverage has increased to 81%

Copy link

github-actions bot commented May 1, 2024

File Coverage Missing
All files 82%
src/pytom_tm/correlation.py 85% 94-95 126
src/pytom_tm/entry_points.py 70% 27 104-121 290-350 439-454 564-580 830-898 939-950
src/pytom_tm/extract.py 73% 110-121 175-182 208-211 283-301
src/pytom_tm/io.py 48% 15-19 25-28 34-37 43-46 52-55 62-66 74-87 93-100 107-114 129-131 184 218-219 261-263 279 295 312-321 341-346
src/pytom_tm/mask.py 80% 70 85 94-95
src/pytom_tm/parallel.py 84% 14-15 81-87 95-97 104 109 148
src/pytom_tm/plotting.py 16% 20-22 25-32 36-44 48-51 54-70 73-74 77-78 98-103 119-127 133-150 165-173 177-194 198-206 210-217 223 229 234 248-398
src/pytom_tm/template.py 57% 57-58 67 76-87 92-97 107-112 114
src/pytom_tm/tmjob.py 97% 276-277 289 295-297 300 432 472
src/pytom_tm/utils.py 78% 16-18
src/pytom_tm/weights.py 96% 61 292-293 300 480 483 486

Minimum allowed coverage is 80%

Generated by 🐒 cobertura-action against 47a0007

@McHaillet
Copy link
Collaborator

@McHaillet I reverted the addition of the poles and responded to your suggestion, please have another look

Error in the test persists :/

@sroet
Copy link
Collaborator Author

sroet commented May 22, 2024

@McHaillet I reverted the addition of the poles and responded to your suggestion, please have another look

Error in the test persists :/

Wait, it wasn't just introduced with the poles? You are sure you are running a clean install of this branch?

@McHaillet
Copy link
Collaborator

I will double check.

@McHaillet
Copy link
Collaborator

Complete fresh install, still happens. No clue. When it tested the branch before holiday here, the tests worked fine. I could try to go back to that commit.

@sroet
Copy link
Collaborator Author

sroet commented May 22, 2024

Complete fresh install, still happens. No clue. When it tested the branch before holiday here, the tests worked fine. I could try to go back to that commit.

Could you also try commit 79efbec and the current main branch?

@McHaillet
Copy link
Collaborator

Complete fresh install, still happens. No clue. When it tested the branch before holiday here, the tests worked fine. I could try to go back to that commit.

Could you also try commit 79efbec and the current main branch?

Exactly, just tried 79efbec and it also fails. I am 100% sure it worked before.

Will also try main.

@McHaillet
Copy link
Collaborator

Will also try main.

Argg, there the tests work

@sroet
Copy link
Collaborator Author

sroet commented May 22, 2024

Alright, so something got triggered in with the changing of the angles.
I am very surprised it is in the volume split test, not the angle split test.

Can you see if those two rotations actually have the same CC score after the volume split, or if we are now somehow catching an artifact of the volume splitting?

@sroet
Copy link
Collaborator Author

sroet commented May 22, 2024

Also, are you using the conda-forge or the pip version of cupy? If you are using the pip version, does it change if you switch to the one on conda-forge?

@sroet
Copy link
Collaborator Author

sroet commented May 22, 2024

I also get an error on the same test, but with a different angle

======================================================================
FAIL: test_tm_job_split_volume (test_tmjob.TestTMJob.test_tm_job_split_volume)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/sander/github_files/pytom-template-matching-gpu/tests/test_tmjob.py", line 301, in test_tm_job_split_volume
    self.assertAlmostEqual(angle_diff, 0, places=1, msg='angle diff should not change')
AssertionError: 192.0 != 0 within 1 places (192.0 difference) : angle diff should not change

----------------------------------------------------------------------
Ran 35 tests in 18.498s

FAILED (failures=1)

@McHaillet
Copy link
Collaborator

I also get an error on the same test, but with a different angle

======================================================================
FAIL: test_tm_job_split_volume (test_tmjob.TestTMJob.test_tm_job_split_volume)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/sander/github_files/pytom-template-matching-gpu/tests/test_tmjob.py", line 301, in test_tm_job_split_volume
    self.assertAlmostEqual(angle_diff, 0, places=1, msg='angle diff should not change')
AssertionError: 192.0 != 0 within 1 places (192.0 difference) : angle diff should not change

----------------------------------------------------------------------
Ran 35 tests in 18.498s

FAILED (failures=1)

Is it the same location?

@McHaillet
Copy link
Collaborator

Also, are you using the conda-forge or the pip version of cupy? If you are using the pip version, does it change if you switch to the one on conda-forge?

Am using the pip version, will give conda forge a try as well

@sroet
Copy link
Collaborator Author

sroet commented May 22, 2024

Is it the same location?

Using the following code:

test_diff = np.abs(
    angle[ok_region, ok_region, ok_region] -
    read_mrc(TEST_ANGLES)[ok_region, ok_region, ok_region])
print(np.argwhere(test_diff > 0.5))

I get

[[40 20  9]]

@McHaillet
Copy link
Collaborator

So also a single location, but different position

@McHaillet
Copy link
Collaborator

Installed the conda forge cupy (its the same version as via pip, i.e. 13.1).

I get the same error and same location as I had before (but different from yours clearly):

[[30 14 26]]

For full clarity, these are my installed package versions:

Package             Version
------------------- -----------
contourpy           1.2.1
cupy                13.1.0
cycler              0.12.1
fastrlock           0.8.2
fonttools           4.51.0
healpix             2024.1
importlib_resources 6.4.0
kiwisolver          1.4.5
matplotlib          3.9.0
mrcfile             1.5.0
numpy               1.26.4
packaging           24.0
pandas              2.2.2
pillow              10.3.0
pip                 24.0
pyarrow             16.1.0
pyparsing           3.1.2
python-dateutil     2.9.0.post0
pytom-match-pick    0.7.0
pytz                2024.1
scipy               1.13.0
seaborn             0.13.2
setuptools          70.0.0
six                 1.16.0
starfile            0.5.8
tqdm                4.66.4
typing_extensions   4.11.0
tzdata              2024.1
voltools            0.6.0
wheel               0.43.0

@sroet
Copy link
Collaborator Author

sroet commented May 22, 2024

So I found a single split in which it does not happen, could you check if this is also a working split for you? (might tell us what is wonky in the algorithm)

Failing    Passing
(2,3,2)    (3,3,2)
(1,3,2)    
(2,1,2)
(2,3,1)
(2,2,2)
(2,4,2)
(2,3,3)

@McHaillet
Copy link
Collaborator

For me its this:

Failing: (2,3,2)
Passing: (1,3,2),(2,1,2),(2,3,1),(2,2,2),(2,4,2),(2,3,3),(3,3,2)

@sroet
Copy link
Collaborator Author

sroet commented May 23, 2024

It becomes weirder...
Doing a complete clean conda install (mimicking our test installation), it now does not fail for (2,3,2) but it does fail for (3,3,2)

@sroet
Copy link
Collaborator Author

sroet commented May 23, 2024

Alright, it the diff seems to be in generated in the actual running of the template matching.

I made sure that every output in the angles and scores matrices is written to once so it is not a race condition in the merging of the data

@McHaillet
Copy link
Collaborator

Alright, it the diff seems to be in generated in the actual running of the template matching.

I made sure that every output in the angles and scores matrices is written to once so it is not a race condition in the merging of the data

Good that the merging is not causing problems.

To sum up:

  • It happens in a (single) specific location that happens only for some splits, but the location does not change with splits
  • The location is not close to any split boundary
  • It varies per machine (?)

My hunch is: at this location the template has two orientations that correlate almost identically. As the splitting changes the volumes very slightly, it occurs sometimes that it changes the correlation there slightly. Could it then just be dependent on the random number generation?

@sroet sroet mentioned this pull request May 24, 2024
4 tasks
@sroet
Copy link
Collaborator Author

sroet commented May 28, 2024

@McHaillet with the refactored volume spitting code in #170 I can't get my test environment to error out for (3,3,2) anymore. Please have another run of it 🤞 :

@McHaillet
Copy link
Collaborator

That sounds promising!! Will test!

@McHaillet
Copy link
Collaborator

McHaillet commented May 28, 2024

Defaults pass, but I tried with different splits as well and then issue is still there:

Current branch:

Pass: (2,2,2), (2,3,2)
Fail: (2,1,2), (1,3,2)

Though overhang was too small perhaps, but with overhang=template.shape similar inconsistency:

Pass: (2,2,2), (2,3,2), (1,3,2)
Fail: (2,1,2)

Also, thinking about it, the benchmark in the test against which the splitting is tested, still appends the full volume to a fast FFT shape. So there is also still that difference...

To sum up, the FFT shape aware splitting is in general a good addition, and at this point we did a lot to fix this tiny inconsistency. I would we say we relax the test for volume splitting to tolerate a small difference.

@sroet
Copy link
Collaborator Author

sroet commented Jun 3, 2024

To sum up, the FFT shape aware splitting is in general a good addition, and at this point we did a lot to fix this tiny inconsistency. I would we say we relax the test for volume splitting to tolerate a small difference.

I commented out the angle diff test (and added a comment why), is that a suitable solution for you?

Copy link
Collaborator

@McHaillet McHaillet left a comment

Choose a reason for hiding this comment

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

I commented out the angle diff test (and added a comment why), is that a suitable solution for you?

That is fine for me!

LGTM! Feel free to merge :)

src/pytom_tm/angles.py Show resolved Hide resolved
@sroet sroet merged commit 584a50f into SBC-Utrecht:main Jun 3, 2024
2 checks passed
@sroet sroet deleted the generate_angles branch June 3, 2024 09:57
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.

angular sampling options
2 participants