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

Organize simSPI's notebooks #105

Open
10 tasks
ninamiolane opened this issue May 2, 2022 · 5 comments
Open
10 tasks

Organize simSPI's notebooks #105

ninamiolane opened this issue May 2, 2022 · 5 comments
Assignees

Comments

@ninamiolane
Copy link
Contributor

ninamiolane commented May 2, 2022

What?

  • Create a tutorial called cryoem_simulation_with_tem_multislice.ipynb that explains the use of the TEM Simulator module
  • Create a tutorial called cryoem_simulation_with_weak_phase_approximation.ipynb that explains the use of the linear simulator.
  • Add corresponding unit-tests.
  • Use pre-commit (see README) to make sure that your code follows style guidelines.

Both of these tutorials should be created by reorganizing elements in existing notebooks (see "How" section below).

Note: This task requires using a docker container to run the TEM simulator.

Why?

Current tutorials on these two modules are hard to understand and use for newcomers, because as of now:

  • the tutorial sim_tutorial.ipynb uses both the TEM simulator and (the noise and shift modules from) the linear simulator without explaining the difference or link between the two.
  • the tutorial tem_tutorial.ipynb also covers the TEM simulator, which can be redundant with part of tutorial sim_tutorial.ipynb.

The library simSPI can only be useful if users can understand it quickly. This task is urgent and very much needed.

Where?

The 2 new tutorials created by this task should go in the ioSPI/notebooks/ folder and be named:

  • cryoem_simulation_with_tem_multislice.ipynb
  • cryoem_simulation_with_weak_phase_approximation.ipynb

The 2 new tutorials are created from existing tutorials that can be found at:

The main modules that the 2 new tutorials should explain are:

How?

The new tutorial cryoem_simulation_with_tem_multislice.ipynb should be created by:

  • extracting the first part of sim_tutorial.ipynb
  • integrating it into the contents of tem_tutorial.ipynb
  • naming the resulting notebook as cryoem_simulation_with_tem_multislice.ipynb

The new tutorial cryoem_simulation_with_weak_phase_approximation.ipynb should be created by:

  • extracting the second part of sim_tutorial.ipynb
  • possibly reformulating the text,
  • naming it cryoem_simulation_with_weak_phase_approximation.ipynb.
@luis-sribeiro
Copy link
Collaborator

luis-sribeiro commented May 2, 2022

Hi @ninamiolane. Should the Projector class be refactored? Right now it is not possible to use it to calculate the projection of an arbitrary volume, instead, by default, the projected volume is assumed to be a cube. And only inside the LinearSimulator class we are able to provide an .mrc file corresponding to the volume, using the init_volume method. It seems more natural to create a Projector object and then use the Projector.forward() method, instead of using LinearSimulator.init_volume() method. What do you think?

Repository owner moved this from Todo to Done in PIMS - Hackathon May 3, 2022
@ninamiolane ninamiolane reopened this May 3, 2022
@ninamiolane
Copy link
Contributor Author

@luis-sribeiro thanks for reaching out!

This specific task is a priori not about refactoring the simulators (nor their projectors), it is about re-organizing the contents of the notebooks. Are you saying that a refactoring of Projector is needed for this task?

@ninamiolane ninamiolane moved this from Done to In Progress in PIMS - Hackathon May 3, 2022
@luis-sribeiro
Copy link
Collaborator

luis-sribeiro commented May 3, 2022

@luis-sribeiro thanks for reaching out!

This specific task is a priori not about refactoring the simulators (nor their projectors), it is about re-organizing the contents of the notebooks. Are you saying that a refactoring of Projector is needed for this task?

Yes, at least for the cryoem_simulation_with_weak_phase_approximation.ipynb notebook, I think it is necessary to refactor Projector. I believe it could flow this way:

  • Exchange the TEMSimulator wrapper section with the new version of Projector, given that in the notebook, the TEMSimulator wrapper it was used only to obtain a projection.
  • Keep the structure where individual functions of the LinearSimulator are used, because they give the user an understanding of what happens in the library under the hood.
  • Add a final session where instead of making the calculations (particle translation, convolution, etc) explicitly, we simply call LinearSimulator.forward() which encapsulates all those operations, and it is how most of the people will use the LinearSimulator.
  • Maybe it is a good idea to spend more time explaining the Config objects in the notebooks. When I first started using the library it was one of the things that got me confused. Sometimes I had to reverse engineer the test files which contained saved dictionaries, in order to correctly populate a Config object.

@ninamiolane ninamiolane added this to the PIMS - Hackathon milestone May 3, 2022
@ninamiolane
Copy link
Contributor Author

Thanks for the details! @bongjinkoo let us know if you encounter the need of Projector refactoring, and if you can follow the steps described by @luis-sribeiro ?

@ninamiolane ninamiolane moved this from In Progress to Todo in PIMS - Hackathon May 4, 2022
@ninamiolane ninamiolane assigned sonyahanson and unassigned bongjinkoo May 6, 2022
@sonyahanson
Copy link
Collaborator

Note that part of this task was accomplished by @bogdantoader tackling issue #107 with PR #119 . There may still be some changes needed to fully comply with the suggestions in this issue, but his notebook micrographs.ipynb explains the use of linear_simulator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

4 participants