Projection/Backprojection Adjointedness

[garrettwrong]

See #1109 .

The following hard coded unit tests would need to be addressed. They are the ones I would expect, no surprises yet. They are all using the LegacyVolume/_LegacySimulation and I have no issue refactoring them.

I'm also wondering if this can resolve the spurious transpose in estimate.

================================================= short test summary info ==================================================
FAILED tests/test_mean_estimator.py::MeanEstimatorTestCase::testAdjoint - NameError: name 'Simulation' is not defined
FAILED tests/test_mean_estimator.py::MeanEstimatorTestCase::testEstimate - NameError: name 'Simulation' is not defined
FAILED tests/test_mean_estimator_boosting.py::test_fsc[volume=<class 'aspire.volume.volume_synthesis.AsymmetricVolume'>, order=None-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
FAILED tests/test_mean_estimator_boosting.py::test_mse[volume=<class 'aspire.volume.volume_synthesis.AsymmetricVolume'>, order=None-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
FAILED tests/test_mean_estimator_boosting.py::test_fsc[volume=<class 'aspire.volume.volume_synthesis.CnSymmetricVolume'>, order=4-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
FAILED tests/test_mean_estimator_boosting.py::test_mse[volume=<class 'aspire.volume.volume_synthesis.CnSymmetricVolume'>, order=4-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
FAILED tests/test_mean_estimator_boosting.py::test_fsc[volume=<class 'aspire.volume.volume_synthesis.CnSymmetricVolume'>, order=5-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
FAILED tests/test_mean_estimator_boosting.py::test_mse[volume=<class 'aspire.volume.volume_synthesis.CnSymmetricVolume'>, order=5-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
FAILED tests/test_mean_estimator_boosting.py::test_fsc[volume=<class 'aspire.volume.volume_synthesis.DnSymmetricVolume'>, order=2-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
FAILED tests/test_mean_estimator_boosting.py::test_mse[volume=<class 'aspire.volume.volume_synthesis.DnSymmetricVolume'>, order=2-resolution=32-dtype=<class 'numpy.float32'>] - AssertionError: 
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! KeyboardInterrupt !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
/Users/gbwright/opt/miniconda3/envs/imgprjadj/lib/python3.8/site-packages/finufft/_interfaces.py:287: KeyboardInterrupt
(to show a full traceback on KeyboardInterrupt use --full-trace)
============================================ 10 failed, 7 deselected in 36.25s =============================================

[garrettwrong]

Need to replace the optimize tests, which were checking a hardcoded call to conj_grad.

I will migrate the mean est and weighted mean est test files towards fixtures while here, since not much is left of the original tests now anyway...

[garrettwrong]

After considering the optimize tests more, I was at a loss how to replace them that would not just be a hardcode mock, and what value that actually brought, so I just culled them. The (conj_grad) code path is tested in any other tests using mean volume estimation.

Otherwise the mean estimator and weighted volume estimator test files were migrated toward pytest fixtures and then extended to cover the usual product of dtype, even-odd pixels, and the two different preconditioner options.

I think the last things left are to resolve this patch for cov3d and self review.

[j-c-c]

Looks good!

[janden]

Looks pretty good. Some minor questions.

On a more general note, should we not have an explicit test that the projection and backprojections are actually adjoints of one another (similar to the type of test that was reported in the issue)?

[garrettwrong]

I've added a direct test of the adjoint relationship to test_mean_estimator. Going to attempt adding a test of src_backward now (previously incorrectly called test_adjoint).

[garrettwrong]

I was able to add a test using the src_backward method. It required scaling by n images. I will look into that further and check the test with a few more situations.

[garrettwrong]

OKay, added src_backward test. Looks like probably need to live with the scaling factor. Not straightfoward way to scale backproject because it only know about the image batch is handling (and they may be different sizes...).

I added a commit (27d89b4) that changes the interface of im_backward in a way that I think is safer. However, that change is totally optional and I'm fine with backing it out. What I was concerned with regarding the current implementation is that is allows for potentially mismatches Image and index relationships, unless I misread it. Can discuss.

[garrettwrong]

Closes #1109

[j-c-c]

Looks good to me.

[garrettwrong]

@janden , reverted the interface change discussed this morning. PR should cover your prior suggestions and be ready for review. Thanks

[janden]

Looks great. Thank1