Multi slice: simulate image from exit wave (apply ctf, detector dqe, ntf, poisson shot noise) #3

geoffwoollard · 2021-08-18T01:00:19Z

I use ioSPI/ioSPI/fourier.py in simSPI/src/multislice.py

I'm not sure how to import fourier into multislice.py, so just put https://github.com/geoffwoollard/simSPI/blob/multi_slice/src/multislice.py#L1 for now

If I have functions to test in simSPI/src/multislice.py

can I put the tests in simSPI/tests/test_multislice.py?
how should I import the functions to test?

Once this PR is working, I can remove fourier.py and other things around. But let's just get things working in simSPI first, and then I'll make some other branches on ioSPI and compSPI and clean things up.

geoffwoollard · 2021-08-18T02:10:19Z

I looked through the deepsource failures. Is there an automated tool for style I can use in git hub?

ninamiolane

Very nice! It would have been better to split this into 2 PRs:

one with the files about the continuous integration
one with the science files.
But this is fine for this one!

I've put some comments but they're very minor.

About DeepSource:

you can use flake8 in your command line to spot deepsource errors in advance
you can use black and isort to automatically correct your files for these errors (isort focuses on cleaning the import order).

It can be that deepsource and the formatting by black or isort

1. are not equal: i.e. deepsource could be more restrictive
1. disagree.
  If this is the case, can you let me know? We would need to clean up the infrastructure of the package in this case.

We don't know at this point if the unit-tests pass, since you have just added the .yml file. When we merge this PR, we should see what they give and be ready to submit a followup HOTFIX" PR if any of the tests fail.

Thank you very much, this is great! 🎉

ninamiolane · 2021-08-18T16:34:09Z

.github/workflows/build.yml

+      matrix:
+        os: [ubuntu-18.04]
+        python-version: [3.7,3.8,3.9]
+        test-folder : ['tests.test_multislice']


test_multisclice.py is a file.py and a folder, thus I don't really know if this will work, but let's try!

there's actually just tests/tests_multislice.py, so I will change it to test-folder : ['tests']. I misread the template yml I was using: https://github.com/compSPI/ioSPI/blob/master/.github/workflows/build.yml#L27

ninamiolane · 2021-08-18T16:35:56Z

src/multislice.py

+    """Exit wave to image (ctf, detector dqe/ntf, and poisson shot noise).
+
+    Convolution of (ctf, detector dqe/ntf,) with exit wave. 
+    Incorporates Poisson shot noise and the collapse of the wave function.


Use the following syntax for references in the docstrings:
https://github.com/geomstats/geomstats/blob/90d9954f525fe6e5b6da625205657675a3baa830/geomstats/geometry/stiefel.py#L161

It allows to render kinda well on the (future) documentation website:
https://geomstats.github.io/geometry.html#geomstats.geometry.stiefel.StiefelCanonicalMetric.inner_product

ninamiolane · 2021-08-18T16:37:51Z

src/multislice.py

+    Parameters
+    ----------
+    exit_wave_f : numpy.ndarray, shape (N,N)
+        exit wave array.


NIT: Uppercase letter on the description of the parameters: we are just enforcing this so that all the docstrings parameters descriptions have the same convention, because that's how the (future) doc website will look clean too.

i.e. Exit wave array.

ninamiolane · 2021-08-18T16:53:39Z

@geoffwoollard another very minor comment : next time, could you put a PR's title and description that are a bit more informative? This will help

the code reviewers to understand a bit better what is happening in the code
the future maintainers to go back to this PR if they need to, by searching through PR's titles.
Thanks!

…n it

…he param

geoffwoollard · 2021-08-18T18:26:35Z

I fixed most of the style issues with black and isort. I think it should be good enough.

I made the PR title more informative.

I still need direction on how to import from ioSPI. I can do a separate PR for that, where I actually move the files around, once I know where everything should go. As things are, I think all the tests will fail, because they rely on fourier from ioSPI, but what I put in (from ioSPI import fourier) won't work.

geoffwoollard · 2021-08-19T02:11:38Z

@fredericpoitevin should I merge the pull request?

ninamiolane · 2021-08-19T02:27:05Z

@geoffwoollard looking great! Let's ignore the deepsource issues:

Assert statement used outside of tests
Missing module/function docstring
but can you address the remaining ones?

ninamiolane · 2021-08-19T02:31:35Z

To import from ioSPI, add a line in the build.yml to import ioSPI from its github. You can find inspiration here:
https://adamj.eu/tech/2019/03/11/pip-install-from-a-git-repository/
in "Installing via Git"

Basically, we want the unit tests to know about the github master of ioSPI, so we need to installing via Git in the build.yml

ninamiolane

Actually, instead of hard-coding the import of ioSPI in the build.yml, you can install it via the simSPI.yml

ninamiolane · 2021-08-19T02:35:29Z

.github/workflows/build.yml

+      - name: install dependencies
+        run:  |
+          # $CONDA is an environment variable pointing to the root of the miniconda directory
+          $CONDA/bin/conda env update --file simSPI.yml --name base


@geoffwoollard you can install ioSPI as a dependenci in simSPI.yml using pip.

So under dependencies in simSPI.yml I'm adding git+https://github.com/comSPI/ioSPI.git, where something like numpy would usually go. This will need me to put in fourier.py into that, and make sure it installs (add in dependencies, etc)... I'll get another PR going for that.

fredericpoitevin

Looking good! I haven't tried to simulate anything yet, but the code is very readable, thanks

fredericpoitevin · 2021-08-19T02:42:35Z

tests/test_multislice.py

+    assert np.allclose(i / high_dose, exit_wave, atol=1e-4)
+
+
+test_exit_wave_to_image()


Is it necessary to execute the function explicitely?

I'll remove that. I see you didn't do that in https://github.com/compSPI/ioSPI/blob/master/tests/test_iotools/test_atomic_models.py

I didn't make it a class...

fredericpoitevin · 2021-08-19T02:52:03Z

Actually, instead of hard-coding the import of ioSPI in the build.yml, you can install it via the simSPI.yml

I meant to work on this before, did not find the time yet. Once we are happy with a mechanism to have the various compSPI repos working with one another, we can document it in compSPI/compSPI.

fredericpoitevin · 2021-08-19T02:56:28Z

@geoffwoollard looking great! Let's ignore the deepsource issues:

Assert statement used outside of tests

Missing module/function docstring
but can you address the remaining ones?

I'm confused, I do not see those issues... See below what I can see:

@geoffwoollard I think those issues could be addressed prior merging.
In any case, I agree with @ninamiolane: thanks for the great contribution, please merge when you are happy :)

geoffwoollard · 2021-08-19T03:08:01Z

I think the assert in tests was removed somehow. I used to see it, but it went away.

I resolved all deepsourse issues that I can see.

ninamiolane · 2021-08-19T04:45:17Z

@geoffwoollard @fredericpoitevin yes I made deepsource ignored these errors automatically

geoffwoollard added 2 commits August 17, 2021 17:46

yml, modified from reduceSPI

cf65234

import fourier from simSPI (not sure if working...

80788f1

geoffwoollard requested review from fredericpoitevin and ninamiolane August 18, 2021 01:00

geoffwoollard added 10 commits August 17, 2021 18:44

add in imports and include raster_geometry for testing in yml

2cb4d8a

import functions to test

4d17f86

4 spaces for indent

427c745

4 spaces for indent, and remove asserts

b61e049

remove unused variables

9136ef6

test all returns

46c8948

docstring indents

de44a37

white space style

75a2be3

style

bf7f94b

bug. varialble name

85d0c33

ninamiolane approved these changes Aug 18, 2021

View reviewed changes

geoffwoollard added 6 commits August 18, 2021 10:40

test-folder fixed to be the folder, not the .py module with testing i…

2c0da88

…n it

format reference

f54c40c

format reference

0349f73

caps in docstring for description of params (the indented one below t…

92a9dab

…he param

blacked for style

e243259

isorted

dc2e111

geoffwoollard changed the title ~~Multi slice~~ Multi slice: simulate image from exit wave (apply ctf, detector dqe, ntf, poisson shot noise) Aug 18, 2021

fourier from iospi

d4aeb1c

ninamiolane reviewed Aug 19, 2021

View reviewed changes

fredericpoitevin approved these changes Aug 19, 2021

View reviewed changes

geoffwoollard added 2 commits August 18, 2021 19:58

style: long line, white space, unused variable

5400455

style: tailing whitespace

a77ff57

geoffwoollard added 3 commits August 18, 2021 20:10

ioSPI from git

93161de

remove call of functions in testing. just define functions

a4d34f4

whitespace

0fcb819

geoffwoollard merged commit 9bbcf3c into compSPI:master Aug 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi slice: simulate image from exit wave (apply ctf, detector dqe, ntf, poisson shot noise) #3

Multi slice: simulate image from exit wave (apply ctf, detector dqe, ntf, poisson shot noise) #3

geoffwoollard commented Aug 18, 2021

geoffwoollard commented Aug 18, 2021

ninamiolane left a comment

ninamiolane Aug 18, 2021

geoffwoollard Aug 18, 2021

ninamiolane Aug 18, 2021

geoffwoollard Aug 18, 2021

ninamiolane Aug 18, 2021

ninamiolane Aug 18, 2021

geoffwoollard Aug 18, 2021

ninamiolane commented Aug 18, 2021

geoffwoollard commented Aug 18, 2021 •

edited

Loading

geoffwoollard commented Aug 19, 2021

ninamiolane commented Aug 19, 2021

ninamiolane commented Aug 19, 2021

ninamiolane left a comment

ninamiolane Aug 19, 2021

geoffwoollard Aug 19, 2021

fredericpoitevin left a comment

fredericpoitevin Aug 19, 2021

geoffwoollard Aug 19, 2021

fredericpoitevin commented Aug 19, 2021

fredericpoitevin commented Aug 19, 2021 •

edited

Loading

geoffwoollard commented Aug 19, 2021

ninamiolane commented Aug 19, 2021

		assert np.allclose(i / high_dose, exit_wave, atol=1e-4)


		test_exit_wave_to_image()

Multi slice: simulate image from exit wave (apply ctf, detector dqe, ntf, poisson shot noise) #3

Multi slice: simulate image from exit wave (apply ctf, detector dqe, ntf, poisson shot noise) #3

Conversation

geoffwoollard commented Aug 18, 2021

geoffwoollard commented Aug 18, 2021

ninamiolane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninamiolane commented Aug 18, 2021

geoffwoollard commented Aug 18, 2021 • edited Loading

geoffwoollard commented Aug 19, 2021

ninamiolane commented Aug 19, 2021

ninamiolane commented Aug 19, 2021

ninamiolane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericpoitevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericpoitevin commented Aug 19, 2021

fredericpoitevin commented Aug 19, 2021 • edited Loading

geoffwoollard commented Aug 19, 2021

ninamiolane commented Aug 19, 2021

geoffwoollard commented Aug 18, 2021 •

edited

Loading

fredericpoitevin commented Aug 19, 2021 •

edited

Loading