Feature/4d service#255
Conversation
This reverts commit 95cd86b.
…TS files from being saved
raphaelpclt
left a comment
There was a problem hiding this comment.
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?
| 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.
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.
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.
@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_simis 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.
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.
| 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.
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
…r submit_data function.
…2 into feature/4d_service
raphaelpclt
left a comment
There was a problem hiding this comment.
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.
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: