-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/4d service #255
Conversation
This reverts commit 95cd86b.
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
…TS files from being saved
There was a problem hiding this 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/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/utils.py
Outdated
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do!
…r submit_data function.
…2 into feature/4d_service
There was a problem hiding this 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.
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer_sim/accufiz_interferometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer/accufiz_interferometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer_sim/accufiz_interferometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer_sim/accufiz_interferometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/accufiz_interferometer_sim/accufiz_interferometer_sim.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
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:
Example testbed script: