Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for applying an external “dusty galaxy” mask map as a configurable preprocessing step prior to other map preprocessing operations.
Changes:
- Introduces a new
GalaxyMaskmap preprocessor that loads/applies a galaxy mask map. - Extends
mask_dustgalto accept an optional logger and emit structured logs. - Updates preprocessor configuration types to include
galaxy_mask, and adjusts the historical lightcurve SLURM wrapper timestamp parsing/header generation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
sotrplib/maps/preprocessor.py |
Adds GalaxyMask preprocessor that loads a mask map via pixell.enmap and combines it with existing masks. |
sotrplib/maps/masks.py |
Updates mask_dustgal signature to accept log and adds logging around mask extraction. |
sotrplib/config/preprocessors.py |
Adds GalaxyMaskConfig and includes it in the preprocessor config union. |
scripts/historical_lightcurve_extractor/slurm_wrapper_depth1_lightcurves.py |
Adjusts timestamp parsing and changes SLURM header generation (removing soconda module loading parameter). |
Comments suppressed due to low confidence (1)
scripts/historical_lightcurve_extractor/slurm_wrapper_depth1_lightcurves.py:172
generate_slurm_header()no longer loads thesocondamodule, but the script still exposes a--soconda-versionCLI option (andget_soconda_module()helper) that are now unused. Either reintroduce the module load behavior (using the CLI arg) or remove the unused CLI option/helper to avoid misleading users into thinking their--soconda-versioninput has an effect.
def generate_slurm_header(jobname, groupname, cpu_per_task, run_dir, slurm_out_dir):
slurm_header = f"""#!/bin/bash
#SBATCH --job-name={jobname} # create a short name for your job
#SBATCH -A {groupname} # group name, by default simonsobs
#SBATCH --nodes=1 # node count
#SBATCH --ntasks=1 # total number of tasks across all nodes
#SBATCH --cpus-per-task={cpu_per_task} # cpu-cores per task (>1 if multi-threaded tasks)
#SBATCH --mem-per-cpu=4G # memory per cpu-core (4G is default)
#SBATCH --time=04:59:00 # total run time limit (HH:MM:SS)
#SBATCH --output={slurm_out_dir}%x.out
export SRUN_CPUS_PER_TASK=$SLURM_CPUS_PER_TASK
cd {run_dir}
source .venv/bin/activate
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def to_preprocessor( | ||
| self, log: FilteringBoundLogger | None = None | ||
| ) -> MapPreprocessor: |
There was a problem hiding this comment.
GalaxyMaskConfig.mask_path defaults to None, but to_preprocessor() always constructs GalaxyMask(mask_path=self.mask_path). With a missing mask_path, the preprocessor will fail at runtime when it tries to read the mask file. Consider making mask_path required (no default) or validate in to_preprocessor() and raise a clear configuration error when it's not provided.
| ) -> MapPreprocessor: | |
| ) -> MapPreprocessor: | |
| if self.mask_path is None: | |
| raise ValueError( | |
| "GalaxyMaskConfig.mask_path must be set to a valid file path " | |
| "before creating a GalaxyMask preprocessor." | |
| ) |
|
|
||
| def preprocess(self, input_map: ProcessableMap) -> ProcessableMap: | ||
| log = self.log.bind(func="GalaxyMask.preprocess") | ||
| if self.mask_map is None: |
There was a problem hiding this comment.
GalaxyMask.preprocess will call enmap.read_map(str(self.mask_path)) when mask_map is None, but mask_path can be None (default). This will try to read a file named "None" and fail with a confusing error. Add an explicit validation/error path when both mask_map and mask_path are unset (e.g., raise ValueError with a clear message, or log and return the input map unchanged).
| if self.mask_map is None: | |
| if self.mask_map is None: | |
| if self.mask_path is None: | |
| raise ValueError( | |
| "GalaxyMask.preprocess requires either 'mask_map' or 'mask_path' to be set." | |
| ) |
allow for using a mask map before map preprocessing, etc.