ffvoigt
commented
4 years ago There are a few issues here, I cannot merge as-is.
- This version does not run, crashes because of the import:
from PyQt5 import QtCore, QtGui, QtWidgets, sipinmulticolor_acquisition_wizard.py-> what is sip good for here? demo_config.pyshould not be changed- I don't understand the way you added a
to_scan-Flag inacquisitions.pyand there are tons of issues with it:- Most importantly this is a nightmare for users: A flag on the very right of the acquisition table dictates whether you scan something or not - easily missed!
- It breaks the way the mesoSPIM software did things so far - if you do not want something to be scanned, delete the row from the table. Why have a two-tier system now?
- What if a user clicks preview on a row flagged as
not to scan- should there be a warning? - "Smart tiling" is not really a good name.
- Clicking a button in the smart tiling window means deselecting it (
not to scanset tofalse) - it is not documented / immediately obvious which way around this is. Shoudn't it be the other way around? - There seems to be a bug that the
not to scan-selection in the smart tiling window is not transferred to the acquisiton manager table - It seems to me that even though a tile is not scanned, an empty file is still created and a metadata file is written - why? That does not make sense.
- The feature that the stages move when you click a tile in the smart tiling window is a nightmare - that is unexpected behavior, never do something like this! You might easily crash a sample / other accidents can happen. Also, why do you need to move to a location anyway for selecting it?
- There is another bug: If you select channels = 1, you never reach the page where you can select an xy/yx order!
- It does not make sense to require zeroing in the middle for the determination of the illumination side
In general, I would say:
- You work indicates that
mesoSPIM-controlneeds better tiling support and shows some steps in the right direction, ideally with some kind of tiling planning window.- It might be a good idea to move away from a wizard function to a tiling window inspired by typical confocal software.
- We might need some community guidelines for code submissions:
- It is better to have more atomic pull requests that do not try to pull off that much - add a single well-tested feature.
Hi Fabian,
I add a new function called "smart tiling". It is in the check tiling page in the tiling wizard. Actually, it is not the "smart" as people conceive now, it just allows users to select tiles that don't have interesting information, so during the tile scanning, these tiles can be skipped.
The other function I added is about a self-determination function of illumination sides in tiling wizard. The idea is that if the zero position of x is put in a line, where the traveling distances of light from left and right side are identical, then the wizard can determine whether the sample should be illuminated from left or right or both side depends on the position of the tile relative to the sample. You can find this choice in the shutter combobox in the general parameters page.
About your comment from last pull request, here is my reply: 1) Now I merge to the dev-better-tiling-wizard, should be fine now. 2) The offset and overlap problem was solved in your original commit of dev-better-tiling-wizard 3) It just offers 6 options now. This 3-combobox interface is similar to what LaVision used, that is the reason I use it. It is easier for users in our lab to understand it. To prevent the confusion you mentioned, I changed the name of the button. 4) Agree. I still have something to add on, we can do it together after that.
One more question, there is a branch about auto-focus. I would like to ask what is the progress of the branch? I would also like to add a function of autofocus in the software, and maybe another function for an automatic search for the ETL parameters.
Tzu-Lun