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

DM-42776: Add star count number to the metadata of bright star stamp and extended PSF models #886

Merged
merged 3 commits into from Feb 8, 2024

Conversation

bazkiaei
Copy link
Contributor

@bazkiaei bazkiaei commented Feb 4, 2024

No description provided.

Comment on lines 557 to 560
self.metadata["psfStarCount"] = 0
for brightStarStamps in bss_ref_list:
stamps = brightStarStamps.get()
self.metadata["psfStarCount"] += len(stamps)
Copy link
Contributor

@leeskelvin leeskelvin Feb 5, 2024

Choose a reason for hiding this comment

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

This produces a change in behavior dependent upon whether the user has requested a region-based ePSF, or a per-detector/per-region based ePSF. That might cause confusion down the line when interpreting this metadata value, requiring an if/else or a try/except, or somehow parsing the detectors_focal_plane_regions parameter further downstream.

My suggestion instead is to also construct a dict here with a single entry. The key can be something simple like "full" or "all" (or something else you feel appropriate), and the value can be this summed number. Then, downstream, we can simply iterate over all psfStarCountDict.items() or psfStarCountDict.values() to get the metadata we need. Does that sound like a good idea here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree with you and it sounds like a good idea! I made modifications based on your comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks Amir, this looks great!

Comment on lines 574 to 577
self.metadata["psfStarCount"][region_name] = 0
for brightStarStamps in ref_list:
stamps = brightStarStamps.get()
self.metadata["psfStarCount"][region_name] += len(stamps)
Copy link
Contributor

Choose a reason for hiding this comment

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

As you did for validStarCount and allStarCount, I'd suggest also adding a small in-line comment here to describe what this psfStarCount code block is doing. (Both here, and replicated above for the non-regions approach).

@bazkiaei bazkiaei merged commit 15edbfe into main Feb 8, 2024
2 checks passed
@bazkiaei bazkiaei deleted the tickets/DM-42776 branch February 8, 2024 22:58
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.

None yet

2 participants