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

Add DM base class. #156

Merged
merged 80 commits into from
Oct 2, 2024
Merged

Add DM base class. #156

merged 80 commits into from
Oct 2, 2024

Conversation

ehpor
Copy link
Collaborator

@ehpor ehpor commented Feb 5, 2024

  • Add ServiceProxy.
  • Add logic for device actuator map.
  • Worry about quantization. Where should that be handled?
  • Add startup map logic (partially already there, but not in the service proxy code).

@ehpor ehpor added the refactor Refactor existing code. label Feb 5, 2024
@ivalaginja
Copy link
Collaborator

ivalaginja commented Mar 25, 2024

Hey @ehpor, I am picking this up as we're at a point on the THD2 implementation that we need the DMs next. I wanted to clarify a couple of points before I put my hand on this (planning a delta PR in case you want to touch this branch here).

In particular, I wanted to ask about the controlled actuators mask. I remember us talking in person about:
(a) the actuators that a BMC DM can control in principle, which on HiCAT is 952
(b) the actuators of the DM that the user chooses to control, which can be a subset of the actuators of (a)

What did we settle on in terms of nomenclature? In #123 (comment), it seems like number (2) in that issue refers to (a) here, but in your PR you use the term "controlled actuators" for this it seems. I know that the concept for (b) is not implemented yet, and you have it on the todo list of this PR, I just wanted to find you in the same place about this before I move on, otherwise it will get very confusing ^^

When referring to (a), do we call those "active" or "controlled" actuators from now on? I like the "controlled" like in your code - if you confirm this, I will make sure to update that catkit2 issue to minimize confusion.

Edit: @ehpor and I decided together that (a) will be called "device actuators" and (b) can be addressed synonymously as "active" or "controlled" actuators.

@ivalaginja
Copy link
Collaborator

ivalaginja commented Apr 2, 2024

I went ahead and updated the comment in #123 (comment) and the one that follows to use "controlled actuators" like in this PR. This means that the todo item in the PR description remains as implementation of "active actuators", which are a subset of the controlled actuators.

See prior comment.

@ivalaginja ivalaginja force-pushed the feature/dm_base_class branch 2 times, most recently from 4d7d473 to f86b864 Compare April 2, 2024 19:45
@ivalaginja
Copy link
Collaborator

@ehpor general question: is this supposed to be a base class for BMC DMs or for DMs in general? I was wondering how to implement the send_surface() command, or to leave it unimplemented.

@ehpor
Copy link
Collaborator Author

ehpor commented Apr 19, 2024

For DMs in general. It's the channels and controlled actuator map that it should abstract away.

@ivalaginja
Copy link
Collaborator

@ehpor another question: do I interpret all your comments correctly, especially in #123, to say that the new service will be usable per each individual DM (e.g. for THD2) as well as per DM pair (like on HiCAT) since all properties are data-driven, but the new proxy will be per pair of DM, like it has been done so far as the proxy can be made data-driven as well?

@ivalaginja
Copy link
Collaborator

ivalaginja commented Apr 23, 2024

@ehpor another question: do I interpret all your comments correctly, especially in #123, to say that the new service will be usable per each individual DM (e.g. for THD2) as well as per DM pair (like on HiCAT) since all properties are data-driven, but the new proxy will be per pair of DM, like it has been done so far as the proxy can be made data-driven as well?

I don't actually think this would work. I tried to implement the conversion from the DM command to BMC (hardware) command (see #123 (comment)) in the last commit (5b5b83a - "Convert DM command to BMC command in send_surface()"), but this makes a hard assumption about all controlled actuators being in the beginning of the hardware command, without any gaps in the array entries. I currently have no idea how to make this work for a pair of DMs - if this was ever the intention.

@ivalaginja
Copy link
Collaborator

I made this work in my delta-PR in #182 for one of the DMs on THD2 (only in simulated mode). However, many questions remain.

@ivalaginja ivalaginja force-pushed the feature/dm_base_class branch 3 times, most recently from ec25f5b to 35c4b0d Compare April 28, 2024 15:01
@ivalaginja
Copy link
Collaborator

ivalaginja commented Apr 28, 2024

@ehpor what I did in the meantime is:

  • Move the discretization into the concrete service since there is no unique way to discretize the DM voltages/surface
  • Discretize the total surface always as well
  • Tried to implement a way to convert to hardware commands that is agnostic to the number of DMs controlled by a single service.

The last point is hard because I have to make assumptions on the type and format of the controlled actuator mask. I remember you talking about stacking those masks into a 3D array, but that doesn't seem possible if the concerned DMs do not have the same shapes. Shall I just stack them into a list instead? This would mean some changes to the proxy, and associated parameters like for example num_actuators and dm_shape.

@ivalaginja
Copy link
Collaborator

ivalaginja commented Apr 28, 2024

@ehpor I just realized you said to concatenate the actuator masks - I suppose into a ravelled 1D array, like we already do with the flatmap and gain map. Is that what you meant?

@ivalaginja
Copy link
Collaborator

ivalaginja commented Jun 20, 2024

Notes from chat at SPIE with @ehpor:

  1. Each DM works on a separate service and separate proxy
  2. Base class should read starting index of hardware command from service config for each DM, array length of the controlled actuators is inferred from the mask read in also from config --> this assumes no gaps in hardware command, exception could be e.g. IrisAO, but not the case for HiCAT right now
  3. dm shape is the literal geometrical shape and can be inferred from the shape of the controlled actuator mask
  4. number of actuators is the sum of the device actuator mask
  5. All input files should be 2D files so they remain human readable - but the attributes in the service should be 3D, always, to indicate number of DMs
  6. Other types of DMs like IrisAO or TT/DAC remain with 1D properties since the geometry is not squares
  7. Controlled actuator mask is always 3D, where 0th index is number of DMs where everything else is whatever shape of the DM --> this only works if you have multiple DMs controlled by the same service, which only works if they all have the same shape (e.g. multiple DACs) and all controlled with the same driver (and also same channels)
  8. Change nomenclature to "device actuators" for physically existing actuators (visual help in fits file) --> this makes it ok to use "controlled" or "active" actuators interchangeably for any subset of the device actuators

@ivalaginja
Copy link
Collaborator

ivalaginja commented Jul 8, 2024

I think I am slowly getting there. The main question I am trying to figure out right now is: What command/map format do we want the proxy method apply_shape() to be able to accept? See this inline comment ffffff for this (@ehpor):

# TODO: with how many potential cases do we want to deal here?

The other thing I will have to check for is whether all of this is compatible with DMs without maps that contain the DM geometry, like a DAC or an IrisAO (see point 6 in this comment further above).

And for now, some notes on the status quo:

  • The device_command_index in the service configuration can be undefined , an integer or a list. In the first case, zero will be assumed. In the second case, only one device is controlled with the service, and in the third case we have several devices of the same number of actuators controlled with the same service and their indices could be different (do we need this)?
  • The data streams now hold exclusively (squeezed) 1D DM commands, which means their length is self.num_actuators * self.num_dms
  • The service has two attributes for the flat map and gain map each: one is a 3D cube of the flat map (gain map) data called self.flat_map (self.gain_map), the other is a 1D DM command called self.flat_map_command (self.gain_map_command and self.gain_map_inv_command) to be able to operate on other DM commands.
  • The service-internal method send_surface() exclusively takes a squeezed DM command (the total surface as read from the incoming data frame from the data stream). This means that if DM maps are used to control the DM, they need to be converted to DM commands in the proxy.

Open Todos:

  • Decide on acceptable input formats of apply_shape() in the new proxy.
  • Double-check that hardware/device command format is indeed only used in hardware communication and nowhere else.
  • Check compatibility with DMs that have non-geometrically encoded DM maps, like a DAC or an IrisAO
  • Test, test, test... in simulation initially.

@ivalaginja ivalaginja force-pushed the feature/dm_base_class branch 3 times, most recently from 1cea757 to 2f5764d Compare July 9, 2024 17:09
@ivalaginja
Copy link
Collaborator

@ehpor do you think you could do an initial review of this, to see whether I am on the right path?

@ivalaginja ivalaginja mentioned this pull request Jul 10, 2024
3 tasks
@ehpor ehpor marked this pull request as ready for review October 1, 2024 22:53
@ehpor ehpor requested a review from ivalaginja October 1, 2024 22:53
Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

@ehpor tested this today in simulation and we tested it with @ehpor , @raphaelpclt and @RemiSoummer on HiCAT on hardware as well. Ready to go.

@ehpor ehpor merged commit cd43bbc into develop Oct 2, 2024
6 checks passed
@ehpor ehpor deleted the feature/dm_base_class branch October 2, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor existing code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants