Create Overview Scan panel from WideFieldAcquisition.py - #66
lizziewylie wants to merge 33 commits into
Conversation
cmeyer
left a comment
There was a problem hiding this comment.
Minor drive-by comments. I'm aware this is draft, btw.
cmeyer
left a comment
There was a problem hiding this comment.
I'm only adding review for coding style, not for functionality or general algorithm logic.
Brow71189
left a comment
There was a problem hiding this comment.
I haven't tried it, just a comment from reading through the code inline below. Apart from that it looks fine.
MattRoyle
left a comment
There was a problem hiding this comment.
Mostly pedantic. Main bits are suggesting that the exporting is done via existing export functionality rather than introducing a new library dependency PIL. The UI line edit handling is done on a per function basis, converting to integers, exiting early if the value is too large. I think using Converters in tandem with property getters and setters would make the code more readable.
MattRoyle
left a comment
There was a problem hiding this comment.
Some issues with the defocus logic and how wide this panel appears.
| display = data_item.display | ||
| display.display_type = "image" | ||
| display_item = display._display_item | ||
| data_path = pathlib.Path(r"C:\AS2\AS2User\Pictures\overview-scan.jpg") |
There was a problem hiding this comment.
If this path already exists then this will override the previous file. It might be useful to allow the flexibility of a UI to name the file with it defaulting to overview-scan with an index incremented to avoid duplicate names overview-scan(1). This way you can switch back to a previous scan without rerunning. In order to properly achieve this you would also need to save the properties json to a file. Then provide a user interface for reloading previously taken overview scans. I think that would be useful but probably something for a follow up.
Fixes #69