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

Tickets/DM-47497 Adding CBP to MTCalsys #177

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from
Draft

Conversation

elanaku
Copy link
Contributor

@elanaku elanaku commented Nov 11, 2024

No description provided.

Copy link
Contributor

@parfa30 parfa30 left a comment

Choose a reason for hiding this comment

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

After reading through all this code and thinking about things a bit more, I think we should add CBP to calib_type but not include CBPCalibration. That can be its own test in mtcalsys.yaml, but I don't think is a whole separate calib_type.

@@ -87,3 +87,20 @@ laser_functional:
use_fiberspectrograph_blue: false
exposure_times:
- 15.0

cbp_calibration:
calib_type: CBPCalibration
Copy link
Contributor

Choose a reason for hiding this comment

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

this is defined as an enum in ts_observatorycontrol/utils/enums.py. calib_type identifies the light source. So let's keep this as Mono and introduce a new variable called use_cbp or something like that.

wavelength: 700
wavelength_width: 800
wavelength_resolution: 50
use_cbpelectrometer: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we should rename the other electrometer to be flatfieldelectrometer or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, use_electrometer is required currently. You'll have to add these two new ones to mtcalsys_schema.yaml

electrometer_range: 2e-6
laser_mode: BURST
optical_configuration: F1_SCU
exposure_times: 30
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know it it will be unhappy unless you put it in this format. It might be fine.
exposure_times:

  • 30.0

@@ -200,6 +206,27 @@ async def setup_calsys(self, sequence_name: str) -> None:
await self.linearstage_projector_select.cmd_moveAbsolute.set_start(
distance=self.ls_select_led_location, timeout=self.long_timeout
)
elif (calibration_type == CalibrationType.CBPCalibration) or (
calibration_type == CalibrationType.CBP
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned above, I thinks CalibrationType should remain the "light input" and we should create a new variable for the CBP vs. FlatField

config_data["cbp_elevation"],
config_data["cbp_mask"],
config_data["cbp_focus"],
config_data["cbp_rotation"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add this to the mtcalsys.yaml file?

rot: `int`
Rotator position of mask in degrees
Default 0
use_projector : `bool`
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this.

self,
azimuth: float,
elevation: float,
mask: typing.Optional[int] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be optional or have a default here? I think every time we move the CBP it would be good to confirm the mask, focus and rotation.

Rotator position of mask in degrees
Default 0
"""
timeout = 60
Copy link
Member

Choose a reason for hiding this comment

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

Please, don't use local timeout variable, we define some default timeouts in the base class. Chose one of those or make a custom one at the class level.

@@ -751,6 +860,7 @@ async def take_electrometer_scan(
self,
exposure_time: float | None,
exposures_done: asyncio.Future,
electrometer_to_scan: salobj,
Copy link
Member

Choose a reason for hiding this comment

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

salobj is a module, not a type. What is the type of this parameter?


Returns
-------
electrometer_exposures : `list`[`str`]
List of large file urls.
"""

self.electrometer.evt_largeFileObjectAvailable.flush()
Copy link
Member

Choose a reason for hiding this comment

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

Why pass in an electrometer if you have it as part of the class already?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants