Logical rather than hardcoded downsample tests

[chris-langfield]

Step 2 of the plan to replace Image.downsample() with the MATLAB ported version.

The current implementation of downsample does not pass all of these.

Step 1 is #565, addition of the crop_2d method required for this algorithm.

#566 #140

[codecov]

Codecov Report

Merging #567 (25b5914) into develop (b000a87) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #567   +/-   ##
========================================
  Coverage    86.49%   86.49%           
========================================
  Files          108      108           
  Lines         8277     8277           
========================================
  Hits          7159     7159           
  Misses        1118     1118           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[chris-langfield]

My reasoning for removing the following test:

ASPIRE-Python/tests/test_starfile_stack.py

Lines 64 to 76 in 3223d6c

	def testImageDownsampleAndWhiten(self):
	self.src.downsample(16)
	self.src.whiten(noise_filter=ScalarFilter(dim=2, value=0.02450909546680349))
	first_whitened_image = self.src.images(0, 1)[0]
	self.assertTrue(
	np.allclose(
	first_whitened_image,
	np.load(
	os.path.join(DATA_DIR, "starfile_image_0_whitened.npy")
	).T, # RCOPT
	atol=1e-6,
	)
	)

is that the processes involved are covered by the new downsampling tests added in this PR, and that the whitening seems to be robustly tested here:

ASPIRE-Python/tests/test_preprocess_pipeline.py

Lines 113 to 150 in 3223d6c

	def testWhiten(self):
	noise_estimator = AnisotropicNoiseEstimator(self.sim)
	self.sim.whiten(noise_estimator.filter)
	imgs_wt = self.sim.images(start=0, num=self.n).asnumpy()
	# calculate correlation between two neighboring pixels from background
	corr_coef = np.corrcoef(
	imgs_wt[:, self.L - 1, self.L - 1], imgs_wt[:, self.L - 2, self.L - 1]
	)
	# correlation matrix should be close to identity
	self.assertTrue(np.allclose(np.eye(2), corr_coef, atol=1e-1))
	def testWhiten2(self):
	# Excercises missing cases using odd image resolutions with filter.
	# Relates to GitHub issue #401.
	# Otherwise this is the same as testWhiten, though the accuracy
	# (atol) for odd resolutions seems slightly worse.
	L = self.L - 1
	assert L % 2 == 1, "Test resolution should be odd"
	sim = Simulation(
	L=L,
	n=self.n,
	unique_filters=[
	RadialCTFFilter(defocus=d) for d in np.linspace(1.5e4, 2.5e4, 7)
	],
	noise_filter=self.noise_filter,
	dtype=self.dtype,
	)
	noise_estimator = AnisotropicNoiseEstimator(sim)
	sim.whiten(noise_estimator.filter)
	imgs_wt = sim.images(start=0, num=self.n).asnumpy()
	corr_coef = np.corrcoef(imgs_wt[:, L - 1, L - 1], imgs_wt[:, L - 2, L - 1])
	# Correlation matrix should be close to identity
	self.assertTrue(np.allclose(np.eye(2), corr_coef, atol=2e-1))

We can probably do the test without hardcoding if it's felt that we should keep it.

[chris-langfield]

Summary of changes @j-c-c :

1. ➕ Add test_downsample.py for the main logical tests. This tests 4 cases (odd-even, odd-odd, etc.) with two different tests. First the "center" gridpoint of the image should match the center of the downsampled image. Then the overall signal energy normalized to the resolution should be approximately conserved. The current downsampling method does not pass this test for one case, but the new method will.
2. 🔁 Change testDownsample in test_Volume. Instead of hardcoded test add a test with the same logic as in 1). In principle this could be extended to more cases as in 1), but since Volume.downsample() uses Image.downsample() under the hood anyway, I don't believe it's necessary.
3. ➖ Remove test02Downsample from test_preprocess.py. This hardcoded test should be covered by the tests in 1).
4. ➖ Remove testDownsample from test_preprocess_pipeline.py. This logical test was expanded and turned into the tests in 1).
5. ➖ Remove hardcodedtestImageDownsampleAndWhiten from test_starfile_stack.py. The downsampling logic test should be covered by the tests in 1), while there is a logical test for the whitening logic in testWhiten in test_preprocess_pipeline.py.
6. Leave testImageDownsample in test_starfile_stack.py where it is (right above the test in 5)). I think this test makes sense because it tests the ImageSource.downsample() method specifically, and its behavior, not the downsample algorithm itself.

[j-c-c]

Is atol related to the total energy in some way? Just wondering why 1e-3.

[chris-langfield]

Absolute tolerance level for allclose. The default is 1e-8, but I had to bump up the tolerance for our downsample method to pass the test.

[janden]

If we use smooth Gaussian instead, we should get better energy conservation since we have much less energy in the high frequencies. It may make sense to do this (i.e., generate volumes using gaussian_blobs instead of loading this ribosome).

[j-c-c]

Looks good. Just a couple questions.

[j-c-c]

Looks good!

[janden]

As I mentioned in the last comment, it may make sense to take this one step further and replace the ribosome volume with Gaussian blobs (that are sufficiently smooth). That should get rid of the loose tolerances since the energy will be preserved.

[janden]

If we use smooth Gaussian instead, we should get better energy conservation since we have much less energy in the high frequencies. It may make sense to do this (i.e., generate volumes using gaussian_blobs instead of loading this ribosome).

[janden]

LGTM

[chris-langfield]

LGTM

We're still using the ribosome file as the test volume for the 3D downsampling. I was working on trying to use 3D Gaussians (but was having a lot of trouble 😁). Did you want to merge this as-is or should I continue?

[chris-langfield]

Could also break it out into a new issue in the interest of getting this downsampling method fully in

[janden]

Sorry got a bit trigger-happy this morning. I'm fine moving it into a separate issue. As a rule, we should be moving to these Gaussian phantoms (away from fixed maps, such as the ribosome).

[chris-langfield]

Sorry got a bit trigger-happy this morning. I'm fine moving it into a separate issue. As a rule, we should be moving to these Gaussian phantoms (away from fixed maps, such as the ribosome).

Sounds good!

[chris-langfield]

Created #600

Re requesting because there was a conflict

[janden]

Sorry for the delay. Merge ahead!