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/perfect estimator #121

Closed
wants to merge 5 commits into from
Closed

Conversation

steigersg
Copy link
Contributor

See also affiliated PR on hicat-package2 (https://github.com/spacetelescope/hicat-package2/pull/470)

Soliciting feedback - contains support for the electric field camera service

@steigersg steigersg added the enhancement New feature or request label Oct 3, 2023
@steigersg steigersg requested review from ehpor and raphaelpclt October 3, 2023 14:03
@steigersg steigersg self-assigned this Oct 3, 2023
Comment on lines +51 to +55
if was_acquiring:
# Ignore first two frames to ensure the frames were taken _after_ the
# call to this function. This is not necessary if the acquisition just
# started.
first_frame_id += 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is relevant in simulation.

Comment on lines +364 to +378
def get_camera_e_field(self, camera_name):
'''Get the current electric field on a camera.

Parameters
----------
camera_name : string
The name of the camera.

Returns
-------
hcipy.Field
The incident power on that camera.
'''
raise NotImplementedError()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to add this function here. Having it only in hicat-simulator.py should be sufficient.

Comment on lines +395 to +410
def camera_e_field_readout(self, camera_name, integrated_e_field):
'''Read out a camera.

This function should be overriden by the child class. It is responsible
for handling the image and submitting it to the right service.

Parameters
----------
camera_name : string
The name of the camera.
integrated_e_field : hcipy.Field
The integrated electric field for this camera, ie. integration time times average
incident electric field on each pixel.
'''
pass

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same - I'm not sure this should live there. Well the question is: do we want this to be a standard catkit feature? If no, having it only in hicat-simulator.py should be good.

Comment on lines +129 to +132
self.camera_integrated_e_field[camera_name] += camera_e_field_images[camera_name] * integration_time
except KeyError:
self.camera_integrated_e_field[camera_name] = (camera_e_field_images[camera_name]
* integration_time)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't integration time be sqrt(integration time)?

@steigersg steigersg closed this Oct 13, 2023
@steigersg steigersg deleted the feature/perfect_estimator branch October 13, 2023 17:48
@steigersg
Copy link
Contributor Author

Closing this PR. Decided for the perfect estimator that we no longer want to use an electric field camera that sends the e-field over a data stream, but instead want to spin up a separate optical model that is updated with the testbed state. Not only is this more self contained and less complex, but it should yield the same results. There could be future use for an e-field camera but it is de-scoped from this PR (still see https://github.com/spacetelescope/hicat-package2/pull/470)

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