General instrument by axf295 · Pull Request #136 · simonsobs/sotrplib

axf295 · 2026-02-05T22:09:12Z

allow for other instruments than just SO LAT; including SO SAT.

I would prefer to use the observatories config files rather than a dictionary in utils.py

…fwhm to decide that

Copilot

Pull request overview

This pull request adds support for the SO SAT instrument alongside the existing SO LAT instrument. The changes enable the pipeline to handle different instruments with varying frequency bands and beam characteristics.

Changes:

Added instrument parameter to get_frequency() and get_fwhm() utility functions, with SOSAT-specific frequency and FWHM values
Updated FWHM-based cuts to use multiples of nominal FWHM instead of absolute arcmin values, improving adaptability across instruments with different beam sizes
Added thumbnail_half_width parameter to blind search configuration for flexible thumbnail sizing
Removed approximately 630 lines of dead code from crossmatch.py (ACT-specific crossmatching functions)

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
sotrplib/utils/utils.py	Added instrument parameter to get_frequency() and get_fwhm() with SOSAT-specific values
sotrplib/sources/forced_photometry.py	Propagated instrument parameter to get_fwhm() call
sotrplib/sources/force.py	Propagated instrument parameter to get_fwhm() calls
sotrplib/sources/blind.py	Added configurable thumbnail_half_width parameter
sotrplib/sims/sim_maps.py	Propagated instrument parameter to get_fwhm() calls
sotrplib/sifter/crossmatch.py	Changed FWHM cuts to use multiples of nominal FWHM; removed ~630 lines of unused ACT-specific code; improved error handling in get_cut_decision
sotrplib/sifter/core.py	Updated default cuts to use FWHM multiples
sotrplib/maps/preprocessor.py	Propagated instrument parameter to get_frequency() and get_fwhm() calls
sotrplib/maps/maps.py	Propagated instrument parameter to get_fwhm() call
sotrplib/filters/filters.py	Removed extraneous blank line
sotrplib/config/blind_search.py	Added thumbnail_half_width configuration option

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sotrplib/sifter/crossmatch.py

Copilot · 2026-02-05T22:16:17Z

sotrplib/utils/utils.py

+    if instrument is not None and instrument.upper() == "SOSAT":
+        frequency = {
+            "f090": 93 * u.GHz,
+            "f150": 145 * u.GHz,
+            "f220": 225 * u.GHz,
+            "f280": 280 * u.GHz,
+        }


The SOSAT frequency dictionary is missing entries for "f030" and "f040" bands that are present in the SOLAT dictionary. If SOSAT instrument uses these frequency bands, they should be included. If SOSAT doesn't support these bands, then calling get_frequency with freq="f030" or freq="f040" and instrument="SOSAT" will raise a KeyError, which may not provide a clear error message to users. Consider either adding these bands (if they're supported) or adding explicit error handling with a clear message about which bands are supported for each instrument.

Copilot · 2026-02-05T22:16:17Z

sotrplib/config/blind_search.py

        return SigmaClipBlindSearch(
            parameters=self.parameters,
            pixel_mask=self.pixel_mask,
+            thumbnail_half_width=self.thumbnail_half_width,
            log=log,
        )


When thumbnail_half_width is None in the config (line 41), passing it to SigmaClipBlindSearch will override the default value of 0.1 deg defined in the constructor (blind.py lines 44-46). This means the default won't be used when the config value is explicitly None. Consider using thumbnail_half_width=self.thumbnail_half_width or AstroPydanticQuantity(u.Quantity(0.1, "deg")) to ensure the default is applied when the config value is None.

Suggested change

return SigmaClipBlindSearch(

parameters=self.parameters,

pixel_mask=self.pixel_mask,

thumbnail_half_width=self.thumbnail_half_width,

log=log,

)

kwargs = {

"parameters": self.parameters,

"pixel_mask": self.pixel_mask,

"log": log,

}

if self.thumbnail_half_width is not None:

kwargs["thumbnail_half_width"] = self.thumbnail_half_width

return SigmaClipBlindSearch(**kwargs)

Copilot · 2026-02-05T22:16:18Z

sotrplib/sifter/crossmatch.py

+    if "fwhm" in cuts:
+        cuts["fwhm_ra"] = [cuts["fwhm"][0] * fwhm, cuts["fwhm"][1] * fwhm]
+        cuts["fwhm_dec"] = [cuts["fwhm"][0] * fwhm, cuts["fwhm"][1] * fwhm]


The cuts dictionary is being mutated in-place by adding fwhm_ra and fwhm_dec keys. If the same dictionary is reused across multiple function calls, these computed values will persist and potentially cause unexpected behavior. Consider creating a copy of the cuts dictionary before modifying it, or use a separate variable for the computed cuts.

Copilot · 2026-02-05T22:16:18Z

sotrplib/sifter/crossmatch.py

+    if i want a source iwth measured fwhm >0.5  and less than 5 times nominal fwhm,
    cuts={'fwhm':[0.5,5.0]}
    cut = (candidate['fwhm']<cuts['fwhm'][0]) | (candidate['fwhm']>cuts['fwhm'][1])



The example in the docstring refers to 'fwhm' as a cut key and suggests cuts={'fwhm':[0.5,5.0]}, but with the new implementation (lines 238-240), the 'fwhm' key is converted to 'fwhm_ra' and 'fwhm_dec' keys with values in arcmin units (multiplied by the nominal fwhm). The example should be updated to clarify that 'fwhm' cuts are specified as multiples of the nominal FWHM (e.g., cuts={'fwhm':[0.5,5.0]} means 0.5x to 5x the nominal FWHM).

Copilot · 2026-02-05T22:16:18Z

sotrplib/sources/blind.py

        for source in extracted_sources:
            ## want to extract the thumbnail at the map location, but then apply ra,dec offsets
-            source.extract_thumbnail(input_map, reproject_thumb=True)
+            ## set thumbnail radius to 0.1 deg by default, but for large fwhm do 5*fwhm (i.e. for SAT it will be ~1.5 deg)


The comment states "for large fwhm do 5*fwhm (i.e. for SAT it will be ~1.5 deg)" but the code doesn't implement this dynamic sizing logic. The thumbnail width is always self.thumbnail_half_width regardless of the instrument's FWHM. Consider either implementing the dynamic sizing mentioned in the comment (similar to the logic in crossmatch.py line 413), or updating the comment to accurately reflect the current implementation where thumbnail_half_width is a fixed configurable parameter.

Suggested change

## set thumbnail radius to 0.1 deg by default, but for large fwhm do 5*fwhm (i.e. for SAT it will be ~1.5 deg)

## thumbnail radius is set by the configurable thumbnail_half_width parameter (default 0.1 deg)

sotrplib/utils/utils.py

sotrplib/sifter/crossmatch.py

Copilot · 2026-02-05T22:16:19Z

sotrplib/maps/preprocessor.py

            maskfile=self.maskfile,
            source_mask=input_map.mask,
-            beam_fwhm=get_fwhm(input_map.frequency),
+            beam_fwhm=get_fwhm(input_map.frequency, instrument=input_map.instrument),


The arr parameter is not being passed to get_fwhm, while it is being passed in other calls throughout the codebase (e.g., in force.py lines 149-153 and maps.py lines 86-90). For consistency and to support potential per-array FWHM values in the future, consider passing input_map.array as the arr parameter.

Suggested change

beam_fwhm=get_fwhm(input_map.frequency, instrument=input_map.instrument),

beam_fwhm=get_fwhm(

input_map.frequency,

instrument=input_map.instrument,

arr=input_map.array,

),

Copilot · 2026-02-05T22:16:19Z

sotrplib/maps/preprocessor.py

            ivarmap=input_map.inverse_variance / input_map.intensity_units.to(u.K) ** 2,
-            band_center=get_frequency(input_map.frequency),
+            band_center=get_frequency(
+                input_map.frequency, instrument=input_map.instrument


The arr parameter is not being passed to get_frequency. For consistency with the API design and to support potential per-array frequency values in the future, consider passing input_map.array as the arr parameter, similar to how it's done in other calls throughout the codebase.

Suggested change

input_map.frequency, instrument=input_map.instrument

input_map.frequency,

instrument=input_map.instrument,

arr=input_map.array,

Allen M. Foster added 3 commits February 5, 2026 16:07

allow generalized instrument, and dont hardcode thumbnail sizes. use …

a398589

…fwhm to decide that

update cuts to unitless fwhm, which is then applied as fraction of fwhm

51a82cb

default cuts are fwhm as relative unitless

c2319a8

axf295 requested a review from Copilot February 5, 2026 22:09

Copilot started reviewing on behalf of axf295 February 5, 2026 22:10 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General instrument#136

General instrument#136
axf295 wants to merge 3 commits intomainfrom
general_instrument

axf295 commented Feb 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-        return SigmaClipBlindSearch(
-            parameters=self.parameters,
-            pixel_mask=self.pixel_mask,
-            thumbnail_half_width=self.thumbnail_half_width,
-            log=log,
-        )
+        kwargs = {
+            "parameters": self.parameters,
+            "pixel_mask": self.pixel_mask,
+            "log": log,
+        }
+        if self.thumbnail_half_width is not None:
+            kwargs["thumbnail_half_width"] = self.thumbnail_half_width
+        return SigmaClipBlindSearch(**kwargs)

	## set thumbnail radius to 0.1 deg by default, but for large fwhm do 5*fwhm (i.e. for SAT it will be ~1.5 deg)
	## thumbnail radius is set by the configurable thumbnail_half_width parameter (default 0.1 deg)

Conversation

axf295 commented Feb 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant