Skip to content

C++ deformable mirror base class#329

Merged
ehpor merged 6 commits into
developfrom
feature/deformable_mirror_base
Jun 12, 2025
Merged

C++ deformable mirror base class#329
ehpor merged 6 commits into
developfrom
feature/deformable_mirror_base

Conversation

@ehpor

@ehpor ehpor commented May 25, 2025

Copy link
Copy Markdown
Collaborator

Work in progress. Todo:

  • Add startup maps.
  • Adding and updating data streams.
  • Add properties.
  • other things?

This PR adds a C++ base class for deformable mirrors. This allows us to perform multiple channel updates asynchronously and independently, unlike the current Python base class. This base class handles channels, initialization of channels and updating from channels, feeding into a generic UpdateSurface() function call, to be implemented by child classes. Rather than adding all channels directly, this class performs delta updates on the channels, allowing calculation of the delta to be performed asynchronously between channels. Afterwards, this delta has to be applied synchronously to the DM.

Note

Due to floating point arithmetic, the delta updates will not be the same as absolute updates (which would add all channels explicitly), and errors will accumulate over time. However, the errors are negligible. Let's do a computation to figure out how much. With 64-bit numbers we have a 52-bit mantissa. This gives a $2^{-53}\approx10^{-16}$ relative error per floating point operation. So, after a month of continuous operation at 10kHz, we would expect a $\sqrt{10000\times60 \times60\times24\times30}\times10^{-16}\approx10^{-11}$ relative error, equal to 36-bit, far less than our DMs.

This PR also adds functionality for reading FITS files to the C++ core.

@ehpor ehpor self-assigned this May 25, 2025
@ehpor ehpor added the enhancement New feature or request label May 25, 2025
@ehpor ehpor requested review from RemiSoummer and steigersg May 27, 2025 17:58
@ehpor

ehpor commented May 27, 2025

Copy link
Copy Markdown
Collaborator Author

This is for incorporating a C++ BMC service, which would simplify #85 (ie. upgrading to be independent of Python version).

@ehpor ehpor marked this pull request as ready for review May 27, 2025 17:59
@ehpor ehpor requested a review from ivalaginja May 27, 2025 22:22
@ivalaginja

Copy link
Copy Markdown
Collaborator

This is exclusively adding code, so the stakes are very low. Have you been able to test this in some way yet or would more in-depth testing happen with a future implementation of a child class?

@ehpor

ehpor commented Jun 5, 2025

Copy link
Copy Markdown
Collaborator Author

It's a generalization refactor of the BMC implementation on SEAL. But I haven't done any testing on this specific version. I'm expecting to find 1-2 bugs/typos when moving over the BMC implementation itself. Same for the ALPAO.

I'm just trying to keep the number of lines limited per PR. ~500 lines is already a lot of C++ code to review, even when it is just adding isolated code.

@ivalaginja ivalaginja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With my limited C++ knowledge, I did my best to review this and see no issues.

When running Copilot on this PR, one returned point stood out, which is: "[...] the status variable is declared but not initialized to zero. According to cfitsio documentation, status must always be set to 0 before passing it to these functions."

This seemed like something to address, but you might be able to tell more immediately than me whether the above is true or not @ehpor.

@ehpor

ehpor commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator Author

@ivalaginja That is 100% correct. I just checked the docs. I'll fix that.

@ehpor

ehpor commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator Author

@ivalaginja Fixed in 941662e.

@ehpor ehpor requested a review from ivalaginja June 11, 2025 23:42

@ivalaginja ivalaginja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@ehpor ehpor force-pushed the feature/deformable_mirror_base branch from 941662e to ef5f1ea Compare June 12, 2025 17:35
@ehpor ehpor enabled auto-merge June 12, 2025 17:35
@ehpor ehpor merged commit 2a225f7 into develop Jun 12, 2025
6 checks passed
@ehpor ehpor deleted the feature/deformable_mirror_base branch June 12, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants