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 47381: Support Summit Observing Weeks 45-46 of 2024 #161

Merged
merged 10 commits into from
Nov 19, 2024

Conversation

edennihy
Copy link
Contributor

No description provided.

@edennihy edennihy requested a review from tribeiro November 18, 2024 19:41
Copy link
Member

@tribeiro tribeiro left a comment

Choose a reason for hiding this comment

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

I have some minor inline comments and one important one. This PR is adding a script (make_cbp_throughput_scan) that should be on its own PR.

@@ -282,6 +295,9 @@ async def configure(self, config: types.SimpleNamespace):
if comp in self.camera.components_attr:
self.log.debug(f"Ignoring Camera component {comp}.")
setattr(self.camera.check, comp, False)
elif comp in ["mtdome", "mtdometrajectory"]:
Copy link
Member

Choose a reason for hiding this comment

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

why make it only mtdome and mtdometrajectory? shouldn't this be

elif comp in self.tcs.components_attr:

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the idea here was that we only wanted to allow the option to ignore the mtdome and mtdometrajectory componets. For example, if a user ignored the mtmount, mtdome, and mtdometrajectory, we were worried it would leave open the option for the telescope to be pointed in the direction of the sun while taking data.

if self.config.point_directly:
if np.abs(az_sun - (self.config.target_az % 360)) < min_sun_distance:
raise RuntimeError(
f"Distance from sun {az_sun - (self.config.target_az % 360)} is \
Copy link
Member

Choose a reason for hiding this comment

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

Please, split the string like we normally do:

f"Distance from sun {az_sun - (self.config.target_az % 360)} is "
f"less than {min_sun_distance}. Stopping."
""

else:
if np.abs(self.config.distance_from_sun) < min_sun_distance:
raise RuntimeError(
f"Distance from sun {self.config.distance_from_sun} is less than {min_sun_distance}. \
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, split the string like we normally do:

f"Distance from sun {self.config.distance_from_sun} is less than {min_sun_distance}. "
"Stopping."

continue
else:
self.log.warning(
f"Calculated exposure time {exp_time} below min exposure time \
Copy link
Member

Choose a reason for hiding this comment

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

Please, split the string like we normally do.

Copy link
Member

Choose a reason for hiding this comment

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

This should not be included as part of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Please do not include new scripts as part of the run branch. The author needs to open a separate PR to include this.

@@ -118,6 +119,14 @@ async def take_images(
reason="EXTRA" + ("" if self.reason is None else f"_{self.reason}"),
program=self.config.program,
)
self.log.info("Waiting for data to be ingested by OODS.")
for _ in range(9):
Copy link
Member

Choose a reason for hiding this comment

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

I think I did this one but we should not have a hard-coded number here. Can you make this a class attribute?

Copy link
Member

Choose a reason for hiding this comment

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

This should not be part of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

This should not be part of this PR.

@edennihy edennihy merged commit 4b23aac into develop Nov 19, 2024
2 checks passed
@edennihy edennihy deleted the tickets/DM-47381 branch November 19, 2024 19:43
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