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

Prepare for waveorder==1.0.0 release #114

Merged
merged 23 commits into from
Apr 18, 2023
Merged

Prepare for waveorder==1.0.0 release #114

merged 23 commits into from
Apr 18, 2023

Conversation

talonchandler
Copy link
Collaborator

@talonchandler talonchandler commented Apr 16, 2023

I am grouping our remaining tasks to prepare for 1.0.0 into this PR. Please feel free to edit or suggest additional tasks.

  • remove WaveorderReader, WaveorderWriter and associated tests from waveorder
  • identify example scripts that we need to maintain - @mattersoflight can you help with this?
  • remove unmaintained examples, and transition the maintained example scripts to iohub and test
  • merge this PR
  • release waveorder 1.0.0

This PR needs to merge before alg-dev merges.

@mattersoflight
Copy link
Member

mattersoflight commented Apr 16, 2023

identify example scripts that we need to maintain - @mattersoflight can you help with this?

Here are the scripts we don't need to maintain in this repo:

Here are the scripts/docs we should maintain:

  • We should maintain all the other forward simulation and reconstruction example as Python scripts. We cannot maintain corresponding jupyter notebooks. They can be replaced with time-stamped PDFs within docs that are updated with every release.
  • The maintained examples can be refactored as the code path evolves. These examples should also be extended with any scripts you use on regular basis.

I'll generate a few PDFs, so you can see where they fit well. I suggest we update them with every release.

Transition the maintained example scripts to iohub and test

Are you thinking of running these examples via pytest? That's an interesting idea and will allow us to test refactors. We should discuss offline which of the tests should be run during CI and which should be run manually before release.

@mattersoflight mattersoflight requested review from edyoshikun and removed request for edyoshikun April 16, 2023 15:48
@mattersoflight
Copy link
Member

@edyoshikun can you help with moving the multiProcessing scripts to recOrder?

@edyoshikun
Copy link

I think we should only have 1 example for the multiprocessing on the recOrder side since the codebase is the same. The current example from @talonchandler here is a great example and the slurm scripts can serve as compliment slurm scripts PR. Also from the testing and future development and refactoring perspective, we agree it is easier to mantain.

@talonchandler
Copy link
Collaborator Author

This PR is ready for review. After discussing with @mattersoflight and looking closely at all of the examples, I partitioned the examples into two sets:

  1. Maintained example scripts that "just work". I have updated scripts that demonstrate QLIPP, 3D phase, and PTI simulations and reconstructions, and I've added them to CI/CD so that we will know when they're failing. I am planning to maintain these scripts as we move through the refactor. All of these are .py files.
  2. experimental_reconstructions notebooks that are dependent on specific datasets, take a longer time to run, and include a lot of duplicated code. I am planning to keep these as a form of documentation, but I am not planning to add these to CI/CD or maintain them through the upcoming refactoring.

@edyoshikun I agree that the multiprocessing example in recOrder is a cleaner version of the work here, so I removed these examples.

I'm very open to suggestions. Does the folder structure makes sense (I'm hoping the new examples/README.md helps)? I haven't added any timestamped PDFs as you suggested, but I think that the notebooks in experimental_reconstructions serve the same purpose?

@talonchandler talonchandler marked this pull request as ready for review April 17, 2023 23:21
mattersoflight
mattersoflight previously approved these changes Apr 18, 2023
Copy link
Member

@mattersoflight mattersoflight left a comment

Choose a reason for hiding this comment

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

Thanks for the reorganization. Looks great!

@talonchandler
Copy link
Collaborator Author

@ziw-liu would you mind taking a quick look at this as well? (Also I accidentally "dismissed" the earlier review by fixing a small problem.)

@ziw-liu when this merges it will be ready for waveorder==1.0.0 release.

examples/README.md Outdated Show resolved Hide resolved
tests/test_examples.py Outdated Show resolved Hide resolved
@talonchandler talonchandler merged commit 5e05b1c into main Apr 18, 2023
@ziw-liu ziw-liu deleted the remove-io branch April 18, 2023 17:17
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.

4 participants