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

Enhancement one #45

Merged
merged 12 commits into from
Jul 21, 2020
Merged

Enhancement one #45

merged 12 commits into from
Jul 21, 2020

Conversation

diegoasua
Copy link
Member

@diegoasua diegoasua commented Jul 15, 2020

This PR brings the following improvements:

@diegoasua diegoasua requested a review from vilim July 15, 2020 15:32
@diegoasua diegoasua self-assigned this Jul 15, 2020
@vilim
Copy link
Member

vilim commented Jul 15, 2020

Keep in mind that once you put in the volume dispatcher, the freeze frame option will be redundant, and there will need to be another way to find which plane you are on (trivial if you ensure the camera only starts triggering once a full piezo cycle is completed and that the first trigger is the beginning. then the 0th plane in the volume will always be known).

self.btn_instructions = QPushButton("User guide")
f = open(r"../../lightsheet_procedure.md")
self.html_markdown = markdown.markdown(f.read())
self.instructions = QTextEdit(self.html_markdown)
Copy link
Member

Choose a reason for hiding this comment

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

make sure this is not editable, so people don't get the mistaken impression that they can update the instructions from the GUI :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed

values=(pulse, pulse + self.state.camera_settings.exposure / 1000),
movable=False,
brush=pg.mkBrush(
166, 196, 240, 100
Copy link
Member

Choose a reason for hiding this comment

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

define these colors as constant tuples at the top of the file, you can unpack them with *

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense

else [
"Seems to follow well current speed"
]
)
)
)

if expected_frame_rate > (triggered_frame_rate * 1.1):
if expected_frame_rate > (frame_rate * 1.1):
Copy link
Member

Choose a reason for hiding this comment

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

this is all just a variable rename, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes just some code refactoring to make it less confusing

@diegoasua
Copy link
Member Author

diegoasua commented Jul 17, 2020

All works, tested in lightsheet rig. Ready to merge. Only annoying thing, there is a warning:

ResourceWarning: Enable tracemalloc to get the object allocation traceback

Due to the instructions popup and the parameters tree. This ResourceWarning is due to having the instructions popup window open. This is Python being worried that something won't get closed. Not important at all (it closes with the software)

@diegoasua
Copy link
Member Author

Can I merge this into master?

Copy link
Member

@vilim vilim left a comment

Choose a reason for hiding this comment

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

All looks good now!

@diegoasua diegoasua merged commit fab530a into master Jul 21, 2020
@diegoasua diegoasua deleted the enhancement_one branch July 21, 2020 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants