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

version 0.3.1: job annotation with module version number, symmetry patch, and whitening filter calculation check #57

Merged
merged 15 commits into from
Nov 3, 2023

Conversation

McHaillet
Copy link
Collaborator

@McHaillet McHaillet commented Nov 2, 2023

Realised that I forgot to apply the symmetry in extraction threshold estimation where it should be used to calculate the full search space.

MUCH more importantly: realised that the angle list was not yet sort and needs to be sorted for effective symmetry reduction!
They are now sorted by the first euler angle upon loading, which should be a general fix that also works for user input angular search.

@McHaillet McHaillet requested a review from sroet November 2, 2023 08:04
@McHaillet
Copy link
Collaborator Author

For some reason this broke something in the unittests. Need to investigate.

@McHaillet
Copy link
Collaborator Author

McHaillet commented Nov 2, 2023

These tests are currently failing:

======================================================================
FAIL: test_search (test_template_matching.TestTM.test_search)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mchaillet/projects/pytom-template-matching-gpu/tests/test_template_matching.py", line 43, in test_search
    self.assertAlmostEqual(stats['std'], 0.005175, places=6,
AssertionError: 0.005174099103974734 != 0.005175 within 6 places (9.008960252659959e-07 difference) : Standard deviation of the search should be almost equal

======================================================================
FAIL: test_parallel_manager (test_tmjob.TestTMJob.test_parallel_manager)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mchaillet/projects/pytom-template-matching-gpu/tests/test_tmjob.py", line 220, in test_parallel_manager
    self.assertTrue(score.max() > 0.934, msg='lcc max value lower than expected')
AssertionError: False is not true : lcc max value lower than expected

======================================================================
FAIL: test_tm_job_split_angles (test_tmjob.TestTMJob.test_tm_job_split_angles)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mchaillet/projects/pytom-template-matching-gpu/tests/test_tmjob.py", line 207, in test_tm_job_split_angles
    self.assertTrue(score.max() > 0.934, msg='lcc max value lower than expected')
AssertionError: False is not true : lcc max value lower than expected

======================================================================
FAIL: test_tm_job_split_volume (test_tmjob.TestTMJob.test_tm_job_split_volume)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mchaillet/projects/pytom-template-matching-gpu/tests/test_tmjob.py", line 175, in test_tm_job_split_volume
    self.assertTrue(score.max() > 0.934, msg='lcc max value lower than expected')
AssertionError: False is not true : lcc max value lower than expected

----------------------------------------------------------------------
Ran 18 tests in 9.616s

FAILED (failures=4)
  1. For the first one I think its safe to reduce to places=6 as it is a very minimal difference.
  2. The other three are the same error: the correlation went down from 0.934 to 0.932 (rounded). The problem is that the correct angle in the test changed as it selects index 100 from the loaded angle list as the ground truth. Since the order of the loaded list changed due to sorting, the angle at index 100 also changed. Interpolation to this position and the added noise gives a small difference. So it's okay to update the expected score.

Less okay about the second point is that this PR will not be backwards compatible. Loading previous angle results from template matching will give different angle indexes in the results. My solution would be to update the README to specify that 0.3.1 is not backwards compatible, because I do think the symmetry is a relevant update and this PR is needed to make it work.

@McHaillet
Copy link
Collaborator Author

Should have thought more about his symmetry option. I think it only works now for templates that are symmetrical around the z-axis. So I updated the input parameter description to specifiy this.

Copy link
Collaborator

@sroet sroet left a comment

Choose a reason for hiding this comment

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

LGTM in general, will approve

The API did not change, so a bugfix version change seems appropriate.

I am not too sure if I understand what is incompatible between the two version. Is it just that the result will be different, or can they not be loaded/analyzed at all?

…r is now only calcualted if not detected in the output dir, otherwise neesds to be recalculated for every job init
@McHaillet McHaillet changed the title Symmetry patch 1 version 0.3.1: job annotation with module version number, symmetry patch, and whitening filter calculation check Nov 3, 2023
@McHaillet
Copy link
Collaborator Author

For my part ready to go. I made the following further updates:

Okay, the version number is now a variable stored in the TMJob structure. When a job is initialized the current module version number is assigned, through pytom_tm.__version__ (which I had to add). When jobs without version numbers are loaded from a json, it must be an older job and therefore they are assigned version '0.3.0', with data.get('version', '0.3.0').

I added a sort_angles option to the angle list loading and in the code this option is True when the job version number is larger than '0.3.0'. I used the version module form packaging to make the check.

For the whitening filter, I added a check to see if the file exists upon job init. This is to prevent recalculation of the filter upon loading a job for particle extraction as it is quite an expensive operation.

Copy link
Collaborator

@sroet sroet left a comment

Choose a reason for hiding this comment

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

You are using a very old/insecure solution for the versioning, please update to use importlib.metadata.version instead

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
src/pytom_tm/__init__.py Outdated Show resolved Hide resolved
src/pytom_tm/angles.py Outdated Show resolved Hide resolved
src/pytom_tm/tmjob.py Outdated Show resolved Hide resolved
@McHaillet
Copy link
Collaborator Author

I switched everything to importlib!

__init__.py now also defines __version__ with this method.

Copy link
Collaborator

@sroet sroet left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM, feel free to merge!

@McHaillet McHaillet merged commit 5dbc519 into SBC-Utrecht:main Nov 3, 2023
@McHaillet McHaillet deleted the symmetry-patch-1 branch November 3, 2023 14:18
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