-
Notifications
You must be signed in to change notification settings - Fork 46
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
Multiple stills view #1463
Multiple stills view #1463
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1463 +/- ##
==========================================
- Coverage 66.67% 66.62% -0.05%
==========================================
Files 616 616
Lines 69187 69228 +41
Branches 9553 9567 +14
==========================================
- Hits 46127 46125 -2
- Misses 21121 21163 +42
- Partials 1939 1940 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments
Co-authored-by: Markus Gerstel <2102431+Anthchirp@users.noreply.github.com>
60c9adb
to
5b78af5
Compare
I would strongly suggest against using force-pushing in pull requests, as it makes it impossible to see what has changed since I reviewed the code (I will not review all of it again). It also breaks up the conversation flow, as it now looks like all commits were made after the code coverage determination and the review comments. This is not what happened. If you want to update the branch you should instead merge |
@Anthchirp , yikes, yea that was not the ideal thing for me to do. The two commits |
@rjgildea can you review? We'd like to know if this is affecting sweeps/grid scans at all. |
…into multiple_stills_view
…into multiple_stills_view
This fixes various bugs that manifest in the image viewer when viewing stills. In particular, combined experiments can now be parsed with reflections placed on the correct image. Further, when paging through multiple images, I noticed that the caching of the dispersion_debug_memo was seemingly broken for my use case, so I simply bypassed it for viewing stills. I think with these commits, the image viewer is functional the way I expect it should be for stills. Maybe its not the ideal way to merge things into master, but at least its a start.
In order to demo the various artifacts that you would experience on the master branch, I designed a script. This will generate a sacla HDF5 file with 5 images:
After running this script you can process the file using stills_process:
Navigate to the output folder, combine the experiments, and view the results
On master branch you will notice all the spots are overlaid in the first image. Further , if you click on threshold view, then page through images, you will notice the display getting stuck, not updating properly.
In this current branch (multiple_stills_view), these problems are fixed, and also the time to launch display of the combined.expt should be quicker.