Feature/ft projector (draft) #124

lederman · 2022-05-10T21:42:38Z

There are a few caveats in this merge.

Technical: I can't run all the tests locally due to configuration issues.
Material:
- the tests are limited (see issue Projection tests? #123 ).
- the output is in the Fourier domain (natural for Fourier projections and followup operations)

Checklist

Verify that your PR checks all the following items.

[x ] The pull request (PR) has a clear and explanatory title.
The description of this PR links to relevant GitHub issues.
[x ] Unit tests have been added in the tests folder:
- [x ] in the test_*.py files corresponding the files modified by this PR,
- [x ] for each function added by this PR.
[x ] The code of this PR is properly documented, with docstrings following simSPI conventions.
The PR passes the DeepSource GitHub Actions workflow (refer to comment below).
The PR passes Test and Lint GitHub Actions workflows. (refer to comment below)

If some items are not checked, mark your PR as draft (Look for "Still in progress? Convert to Draft" on your PR) . Only mark the PR as "Ready for review" if all the items above are checked.

If you do not know how to address some items, reach out to a maintainer by requesting reviewers.

If some items cannot be addressed, explain the reason in the Description of your PR, and mark the PR ready for review

Description

Issue

Additional context

ninamiolane

Thank you very much!

Re DeepSource:
The DeepSource errors can be accessed by clicking on "Details" next to the GitHub workflow called "DeepSource", or equivalently at this link.

I believe the issues found by DeepSource can be addressed. Let us know if you see a blockage?

Re Test:
The Test errors can be accessed by clicking on "Details" next to the GitHub workflow called "Test", or equivalently at this link.

I have copy-pasted the error below, it seems that the projector has been initialized without a space attribute.

I believe this can be fixed too? Let us know if not.

tests/test_linear_simulator.py:79: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
simSPI/linear_simulator/linear_simulator.py:30: in __init__
    self.projector = Projector(config)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = Projector()
config = {'chunks': 4, 'pixel_size': 0.8, 'cs': 2.7, 'amplitude_contrast': 0.1, 'ctf_size': 32, 'noise': False, 'noise_sigma': ...ise_distribution': 'gaussian', 'input_volume_path': '', 'side_len': 32, 'kv': 300, 'value_nyquist': 0.1, 'b_factor': 0}

    def __init__(self, config):
        """Initialize volume grid."""
        super(Projector, self).__init__()
    
        self.config = config
>       self.space = config.space
E       AttributeError: 'AttrDict' object has no attribute 'space'

ninamiolane · 2022-05-10T22:45:36Z

Linking to issue #102

mbrubake · 2022-05-11T14:50:12Z

Hmmm, I just fixed the linear simulator test in my local branch but can't seem to push the change. Any ideas why?

mbrubake · 2022-05-11T18:29:44Z

Ok, got the changes pushed, the linear_simulator test should be fixed now.

codecov · 2022-05-11T18:50:49Z

Codecov Report

Merging #124 (0883aca) into master (b20c05e) will decrease coverage by 0.45%.
The diff coverage is 89.19%.

❗ Current head 0883aca differs from pull request most recent head ffea6ab. Consider uploading reports for the commit ffea6ab to get more accurate results

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
- Coverage   98.39%   97.94%   -0.44%     
==========================================
  Files          15       15              
  Lines         744      776      +32     
==========================================
+ Hits          732      760      +28     
- Misses         12       16       +4

Impacted Files	Coverage Δ
simSPI/linear_simulator/projector.py	`92.31% <89.19%> (-7.69%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b20c05e...ffea6ab. Read the comment docs.

ninamiolane

LGTM, thank you! I made minor comments.

ninamiolane · 2022-05-11T23:23:53Z

simSPI/linear_simulator/projector.py

+
+            self.register_buffer("vol_coords", coords.reshape(-1, 3))
+        elif self.space == "fourier":
+            # Assume DC coefficient is at self.vol[n//2+1,n//2+1]


This comment could be moved in a docstring, so that it is more visible by users --- when we will publish the documentation website with Sphinx.

This relates a bit to my comment in issue #102, we should try to standardize somewhere the conventions for Fourier space representations. Then it doesn't really need to be anywhere... I'm also not sure that this is currently accurate.

ninamiolane · 2022-05-11T23:26:14Z

simSPI/linear_simulator/projector.py

+
+        Parameters
+        ----------
+        rot_params : tensor of rotation matrices


The description of rot_params does not strictly follow the docstring convention:

if it is a tensor, what is its shape

it should be on two lines, where the first line explains the type of datastructure and the second explains what the parameter represents.

This will become important to generate a clean documentation website.

Additionally, this description says that rot_params is a tensor; yet _forward_fourier docstring mentions a dict: which one is true?

Maybe this docstring could point to _forward_fourier docstring about the details? I find it well explained there that rot_params is a dictionary that contains a tensor of specific shape.

ninamiolane · 2022-05-11T23:26:48Z

simSPI/linear_simulator/projector.py

+
+        # rescale the coordinates to be compatible with the edge alignment of
+        # torch.nn.functional.grid_sample
+        if self.config.side_len % 2 == 0:  # even case


The comments even case and odd case might not be necessary as it is evident from the code.

ninamiolane · 2022-05-11T23:28:00Z

simSPI/linear_simulator/projector.py

+            (batch_sz, self.config.side_len, self.config.side_len),
+            dtype=torch.complex64,
+        )
+        # interpolation is decomposed to real and imaginary parts due to torch


Should this be put in a docstring?

ninamiolane · 2022-05-11T23:28:10Z

tests/test_projector.py

+def test_projector_fourier():
+    """Test accuracy of projector function.
+
+    Note: corrent test only checks that the scaling is compatible.


Typo: current

ninamiolane · 2022-05-11T23:28:48Z

tests/test_projector.py

+        torch.fft.fftshift(saved_data["projector_output"], dim=(2, 3))
+    )
+
+    print(out.dtype)


I think we should avoid prints in the tests because our logs might become overcrowded when we add more tests: could these become asserts?

I agree, maybe these could be replaced with logging, so they could be displayed if really needed?

fredericpoitevin

Thanks so much! This is a great addition 🙏

fredericpoitevin · 2022-05-12T00:01:49Z

simSPI/linear_simulator/projector.py

+    def _forward_fourier(self, rot_params):
+        """Output the tomographic projection of the volume in Fourier space.
+
+        Take a slide through the Fourier space volume whose normal is


Typo: slice

fredericpoitevin · 2022-05-12T00:05:13Z

simSPI/linear_simulator/projector.py

+
+        Parameters
+        ----------
+        rot_params : tensor of rotation matrices


Maybe this docstring could point to _forward_fourier docstring about the details? I find it well explained there that rot_params is a dictionary that contains a tensor of specific shape.

fredericpoitevin · 2022-05-12T00:10:08Z

tests/test_projector.py

+        torch.fft.fftshift(saved_data["projector_output"], dim=(2, 3))
+    )
+
+    print(out.dtype)


I agree, maybe these could be replaced with logging, so they could be displayed if really needed?

mbrubake and others added 7 commits May 6, 2022 16:15

First cut implementation of projector in Fourier space.

a25197e

scale grids after rotation

ba72105

scale grids factor fixed

ada366e

redo tests

e854004

test fft scale

062d0dd

style corrections

fde4f09

style corrections

de7c746

lederman requested review from fredericpoitevin, mbrubake and ninamiolane May 10, 2022 21:42

ninamiolane requested changes May 10, 2022

View reviewed changes

mbrubake added 2 commits May 11, 2022 10:39

Merge remote-tracking branch 'origin/master' into feature/ft-projector

22b004f

Update linear_simulator config

92644f4

mbrubake added 6 commits May 11, 2022 14:39

Include space parameter in projector_data.npy

6096255

style fix

d8de646

Some error checking

f511945

Remove elif after return

e8f57ea

Shorten line

6e1e537

Remove commented out code

eb6df3e

Remove whitepsace

ffea6ab

ninamiolane approved these changes May 11, 2022

View reviewed changes

fredericpoitevin approved these changes May 12, 2022

View reviewed changes

Feature/ft projector (draft) #124

Are you sure you want to change the base?

Feature/ft projector (draft) #124

Uh oh!

Conversation

lederman commented May 10, 2022 • edited by ninamiolane Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description

Issue

Additional context

Uh oh!

ninamiolane left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninamiolane commented May 10, 2022

Uh oh!

mbrubake commented May 11, 2022

Uh oh!

mbrubake commented May 11, 2022

Uh oh!

codecov bot commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ninamiolane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericpoitevin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lederman commented May 10, 2022 •

edited by ninamiolane

Loading

ninamiolane left a comment •

edited

Loading

codecov bot commented May 11, 2022 •

edited

Loading