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

feature: tem refactoring + tutorial notebook #87

Merged
merged 64 commits into from
Mar 25, 2022
Merged

Conversation

jedyeo
Copy link
Collaborator

@jedyeo jedyeo commented Mar 10, 2022

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@geoffwoollard
Copy link
Contributor

This looks incomplete. I put some comments in https://app.reviewnb.com/compSPI/simSPI/pull/86/

@@ -0,0 +1,457 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the outputs to the notebook are available/run in the committed version that is publicly available.


Reply via ReviewNB

@@ -0,0 +1,457 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

image not showing up


Reply via ReviewNB

@jedyeo jedyeo marked this pull request as ready for review March 24, 2022 23:03
Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, just one comment about why the descriptions of some parameters have been removed.

defocus_syst_error_um: 0.0 # [μm]
defocus_nonsyst_error_um: 0.0 # [μm]
optics_defocusout: None # file to write defocus values
magnification: 81000 #
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing the descriptions of the parameters?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for catching this. These seem to have been accidentally removed during a merge conflict. The descriptions have been added back.

@geoffwoollard
Copy link
Contributor

Hooray!!!

Look at that beautiful test coverage!

Look back and see how far you've come, enjoy, and imagine how this could come together in the time you have left.

Keep working hard!

---------- coverage: platform linux, python 3.9.12-final-0 -----------
Name                                          Stmts   Miss  Cover   Missing
---------------------------------------------------------------------------
simSPI/__init__.py                                1      0   100%
simSPI/crd.py                                    27      0   100%
simSPI/fov.py                                    76      0   100%
simSPI/linear_simulator/ctf.py                   37      0   100%
simSPI/linear_simulator/linear_simulator.py      33      0   100%
simSPI/linear_simulator/noise_utils.py            8      0   100%
simSPI/linear_simulator/params_utils.py          87      0   100%
simSPI/linear_simulator/projector.py             20      0   100%
simSPI/linear_simulator/shift_utils.py           22      0   100%
simSPI/linear_simulator/volume_utils.py           7      0   100%
simSPI/multislice.py                             31      0   100%
simSPI/tem.py                                   122      5    96%   192-193, 196, 242-246
simSPI/tem_distribution.py                       18      0   100%
simSPI/tem_inputs.py                            203      6    97%   354-355, 384-385, 504-505
simSPI/transfer.py                               52      3    94%   100, 107, [177](https://github.com/compSPI/simSPI/runs/5685549113?check_suite_focus=true#step:4:177)
---------------------------------------------------------------------------
TOTAL                                           744     14    98%
Coverage XML written to file coverage.xml

Required test coverage of 90.0% reached. Total coverage: 98.12%
================= 57 passed, 28 warnings in 637.83s (0:10:37) ==================

Copy link
Member

@fredericpoitevin fredericpoitevin left a comment

Choose a reason for hiding this comment

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

Good work, guys!

Comment on lines +487 to +489
rotation_metadata : array-like, shape=[..., 3]
N x 3 matrix representing the rotation angles , phi, theta, psi, of
each particle in stack.
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to say which convention is used: XYX, ...?

@@ -7,6 +7,7 @@
hole_diameter_nm: 1200 # [nm]
hole_thickness_center_nm: 100 # [nm]
hole_thickness_edge_nm: 100 # [nm]
particle_slice_pad: 5 #
Copy link
Member

Choose a reason for hiding this comment

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

# [pixel] ?

@@ -15,7 +16,7 @@
electron_dose_std_e_per_nm2: 0 # standard deviation of dose per image

optics_parameters:
magnification: 81000 #
magnification: 81000 #
Copy link
Member

Choose a reason for hiding this comment

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

# []

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

fredericpoitevin commented on 2022-03-25T16:47:14Z
----------------------------------------------------------------

Add link to reference paper: https://onlinelibrary.wiley.com/doi/10.1111/j.1365-2818.2011.03497.x


@review-notebook-app
Copy link

review-notebook-app bot commented Mar 25, 2022

View / edit / reply to this conversation on ReviewNB

fredericpoitevin commented on 2022-03-25T16:47:15Z
----------------------------------------------------------------

Some explanatory text would have been helpful.


@fredericpoitevin fredericpoitevin merged commit b66f479 into master Mar 25, 2022
@fredericpoitevin fredericpoitevin deleted the tem/refactor branch March 25, 2022 16:49
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.

6 participants