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

Development #135

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Development #135

wants to merge 31 commits into from

Conversation

xiyuyi
Copy link

@xiyuyi xiyuyi commented Dec 1, 2022

Added extra functions in the Hamamatsu camera module to allow for master pulse start trigger mode.

Xiyu Yi and others added 30 commits June 13, 2022 19:35
added labjack control for 2-axis galvos.
Alignment configuration for two pairs of gamma/beta galvos.
… on hamamatsu 3D acquisition with user choices of parameters.
# Conflicts:
#	sandbox/hamamatsu_camera_try_master_pulse.py
Copy link
Contributor

@AhmetCanSolak AhmetCanSolak left a comment

Choose a reason for hiding this comment

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

Nice first PR @xiyuyi , I dropped multiple comments for you to check and please remove all sandbox files as we don't want them on the main branch. After a few iterations, we can merge some part of this branch and we can divide the rest into multiple other PRs. Thank you!

@@ -2,7 +2,7 @@
from setuptools import setup


if sys.version_info < (3, 9):
if sys.version_info < (3, 8):
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason behind switching to Python3.8?

@@ -0,0 +1,207 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

.ipynb files create huge diffs(as seen here below), so we don't develop our code in notebooks in general @xiyuyi . You can use them locally but please refrain from committing these files. They are practically impossible to review. If you had to have them under version control(which probably you don't have to and happy to discuss each case independently), there are ways to convert these files to more readable/reviewable formats to keep them under version control.

@@ -1,5 +1,4 @@
# coPylot

Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to avoid unnecessary diffs.

from copylot.hardware.orca_camera.dcamapi4 import DCAM_IDPROP


def dcamtest_show_framedata(data, windowtitle, iShown):
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a nice demo function but I think living in the wrong place. Could you move this to a suitable demo file @xiyuyi ?

"""
Show numpy buffer as an image

Arg1: NumPy array
Copy link
Contributor

Choose a reason for hiding this comment

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

We use NumPy style docstrings @xiyuyi , you can set your pycharm to generate NumPy style docstrings and if you like to read more about it see this: https://numpydoc.readthedocs.io/en/latest/format.html


Dcamapi.uninit()

def live_capturing_return_images_get_ready(self, nb_buffer_frames=3, timeout_milisec=100):
Copy link
Contributor

Choose a reason for hiding this comment

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

functions like this one are quite risky. Dcamapi is initialized here and who knows where it is uninitialized, these situations named as side-effects. We don't want them. We want to minimize the side-effects. DCAM API has many concepts that need init/uninit, so I am planning to implement these in a different way. For now you can remove this function and create a issue for the need so we can address it together @xiyuyi .

return self.data

def live_capturing_return_images_capture_end(self):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to my comment above, this is yet another method with multiple undesired side-effects, will be good to remove for now.

self.dcam.buf_release()
Dcamapi.uninit()

def set_configurations(self, camera_configs, camera_ids=[0]):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can refactor this method into something much more readable, let's chat on this in person some time @xiyuyi .

dcam.buf_alloc(self.buffer_size_frame_number - 1)

def get_ready(self, camera_ids=[0]):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

yet another method, that shares similar side-effects.

assert self.devices['camera '+str(camera_id)].is_opened()

def start(self, camera_ids=[0]):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

we will need to refactor this method and the following methods that shares similar purpose.

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.

2 participants