Skip to content

Conversation

@suberlak
Copy link
Contributor

Adding donut_id based on detector and donut brightness.

@suberlak suberlak force-pushed the tickets/DM-52459 branch 3 times, most recently from 6c14e99 to 7ca3af4 Compare November 19, 2025 23:17
@suberlak suberlak requested a review from jbkalmbach November 20, 2025 00:00
Copy link
Member

@jbkalmbach jbkalmbach left a comment

Choose a reason for hiding this comment

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

Why did you decide to make this a string instead of an int?

camera coordinate system (CCS), with the CWFSs rotated to the same
orientation as the science sensors. It is this object that will be used
to interface with the wavefront estimator.
Copy link
Member

Choose a reason for hiding this comment

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

Remove blank line.

bandpass = donutStamp.bandpass
self.assertEqual(bandpass, "r")

donutId = donutStamp.donut_id
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 you could remove the blank line above this since it goes with testing the properties.

Copy link
Member

@jbkalmbach jbkalmbach left a comment

Choose a reason for hiding this comment

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

Do we care that the same donut could have different donut_id values in an extra-intra pair when running in full array mode? Probably not I think but can you think of any reason?

@suberlak
Copy link
Contributor Author

I went along with a str because it allowed a possibility of making names like eg. 191_0001 or R00_191_0001 etc. But I could revert to int if you think it's better. It is possible to fold in a visit number (since I have access to exposure in generateDonutCatalogUtils.py), i.e. make it not only 2025053100117_191_0001, but 202505310011701910001 (eg. 4 spaces for detector and 4 spaces for donut for clearer reading)- that way we'd have visit-detector-donut granularity (i.e. possible to distinguish intra/extra when running in FAM mode since extra/intra have different visit number).

@suberlak suberlak force-pushed the tickets/DM-52459 branch 3 times, most recently from 88aa773 to 1ae9565 Compare December 16, 2025 03:54
@suberlak suberlak requested a review from jbkalmbach December 16, 2025 03:54
Copy link
Member

@jbkalmbach jbkalmbach left a comment

Choose a reason for hiding this comment

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

It looks like you need to add a test somewhere that verifies donut_id is output in the format you expect. I don't see a test for that anywhere.

@suberlak
Copy link
Contributor Author

added test for donut_id

Copy link
Member

@jbkalmbach jbkalmbach left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

@suberlak suberlak merged commit 0b30f24 into develop Dec 18, 2025
4 checks passed
@suberlak suberlak deleted the tickets/DM-52459 branch December 18, 2025 04:35
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