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

Feature/4d service #255

Merged
merged 34 commits into from
Oct 24, 2024
Merged

Feature/4d service #255

merged 34 commits into from
Oct 24, 2024

Conversation

lanemeier7
Copy link
Collaborator

@lanemeier7 lanemeier7 commented Oct 18, 2024

closes #254 and creates an accufiz_interferometer service for catkit2

Requires running 4d Web Services running and 4Sight Focus Software in listening mode when running actual hardware.

example service.yml entry:

accufiz_interferometer:
  service_type: accufiz_interferometer
  simulated_service_type: accufiz_interferometer_sim
  interface: camera
  requires_safety: false
  #sim_data: C:/path/to/example.h5
  mask: C:/path/to/4d.mask
  server_path: C:/path/to/data
  local_path: C:/path/to/data
  ip_address: localhost:8080
  num_avg: 2

Example testbed script:

import pathlib
import os
import matplotlib.pyplot as plt
import numpy as np
from catkit2 import TestbedProxy, read_config_files

config_path = pathlib.Path(os.path.join('tests/', 'config'))
config_files = config_path.resolve().glob('*.yml')
config = read_config_files(config_files)

port = config['testbed']['default_port']

testbed = TestbedProxy('127.0.0.1', port)
cam = testbed.accufiz_interferometer
img = np.mean(list(cam.take_raw_exposures(num_exposures=2)), axis=0)

plt.imshow(img, cmap='inferno')
plt.colorbar()
plt.show()

@lanemeier7 lanemeier7 self-assigned this Oct 18, 2024
@lanemeier7 lanemeier7 requested review from ehpor, steigersg and raphaelpclt and removed request for raphaelpclt October 18, 2024 18:21
Copy link
Collaborator

@raphaelpclt raphaelpclt left a comment

Choose a reason for hiding this comment

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

Very nice! I have a few comments still, on datastream submission, on the relevance of the utils.py file and a doctstring format.

One more general questions still: no close method have been implemented in the service. I have no experience with the 4d at all, but just checking. There's no "sleep mode" or so we would want to put it in while closing the service?

catkit2/utils.py Outdated
Comment on lines 1 to 16
import numpy as np

def rotate_and_flip_image(data, theta, flip):
"""
Converts an image based on rotation and flip parameters.
:param data: Numpy array of image data.
:param theta: Rotation in degrees of the mounted camera, only these discrete values {0, 90, 180, 270}
:param flip: Boolean for whether to flip the data using np.fliplr.
:return: Converted numpy array.
"""
data_corr = np.rot90(data, int(theta / 90))

if flip:
data_corr = np.fliplr(data_corr)

return data_corr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hum - I think this might overlap at some point with #124. The general idea is that camera flips should be handled at a very low level, and then not touched anymore. And this should be taken care of by a coming camera base class. So even if this function is very relevant for you, I don't think it should be part of a utils file in the catkit2 context.
TLDR; I'd prefer this to be somewhere else - e.g. as static method in the service? @ehpor @ivalaginja @steigersg any opinion?

Copy link
Collaborator Author

@lanemeier7 lanemeier7 Oct 23, 2024

Choose a reason for hiding this comment

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

I have no problem making it a static method. I saw this was a util in catkit1 so I copied this policy.
My 2 cents is that having static methods inside services makes for a lot of copy pasted code and violation of the don't repeat yourself rule.

On a slightly separate but similar topic, it would be nice if the simulated service modules could inherit the non -simulated hardware service classes and just override the stuff that needs to be different (like fake communication or fake data generation). I quickly tried this and things were unhappy in my first attempt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lanemeier7 Catkit1 did exactly this. We consciously chose not to do this for catkit2, for two reasons:

  • The lever of overriding is an important consideration. Catkit1 mocked the actual API coming from the hardware supplier. This is the lowest level of mocking that we can reasonably do. However, some of the low-level APIs are quite extensive and require a large amount of work to fully override. Catkit2 however mocks at the service level. This allows using one simulated service for multiple hardware services, if they are similar enough (see for example the camera, where camera_sim is the simulated service for now three hardware camera services.
  • We want to strictly avoid loading of hardware libraries during imports. This allows for running of simulations on machines that do not have hardware APIs installed.

We can still do what you are asking for, in a separate way. Take a look at how the bmc_deformable_mirror_hardware and *_sim services are implemented. They use a common service base that implements common code between simulated software and hardware.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As for this function itself: I personally don't see a reason to have a function for effectively two lines of code, each executing a single Numpy function. Many people will be aware of the Numpy function, but not of the util function. So those people would likely use the two numpy functions, rather than use this util function. So, unless I was using this function >3-5 times inside my service, I'd likely not have it be a function at all, if I was writing this.

That said, I totally concede that this is largely subjective and my personal preference, so I'd be fine with anything.

catkit2/utils.py Outdated
Comment on lines 5 to 9
Converts an image based on rotation and flip parameters.
:param data: Numpy array of image data.
:param theta: Rotation in degrees of the mounted camera, only these discrete values {0, 90, 180, 270}
:param flip: Boolean for whether to flip the data using np.fliplr.
:return: Converted numpy array.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use the numpy docstring format (some non-compliant docstrings might exist in the repos, but we try to get rid of them) - could you change to that format?
https://numpydoc.readthedocs.io/en/latest/format.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can do!

Copy link
Collaborator

@raphaelpclt raphaelpclt left a comment

Choose a reason for hiding this comment

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

Almost there!
Thank you so much for all the docstring - and I feel bad requesting so many changes. But it would be nice if all the docstring would match the numpy style (it seems you have the google style). I've put one example. And you can probably set up your code editor if that can help.

Copy link
Collaborator

@raphaelpclt raphaelpclt left a comment

Choose a reason for hiding this comment

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

Looks good.

@lanemeier7 lanemeier7 merged commit f90c03f into develop Oct 24, 2024
6 checks passed
@lanemeier7 lanemeier7 deleted the feature/4d_service branch October 24, 2024 17:11
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.

Add service for 4D AccuFiz interferometer
4 participants