Enforce waveform minimum frequency location in v0.7 (#107)

* Initial plan

* Add minimum frequency field to Waveform and update validation

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Update config templates to use waveform minimum frequency

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Move validation before pipeline init and improve error handling

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Fix meta_defaults access in validation code

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Fix type definition and add waveform variable to templates

- Change minimum_frequency type to dict[str, float] only (remove float option)
- Add waveform variable assignment to bilby.ini and lalinference.ini templates
- Use waveform shorthand in lalinference.ini for consistency

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Add robust error handling for minimum frequency validation

- Add type checking in bayeswave.py flow property
- Add type checking in pesummary.py for both SubjectAnalysis and single analysis paths
- Add type checking in pipeline.py PESummaryPipeline
- Ensure all code paths validate that minimum frequency is a non-empty dict before calling .values()

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Split external blueprint tests to separate CI job

- Create local copies of external blueprints with correct v0.7 waveform structure
  in tests/test_data/blueprints/
- Refactor test_specific_events.py to use local blueprints (must pass) with
  __file__-based absolute paths to be robust against cwd pollution from other tests
- Add tests/external_blueprint_compat.py for external URL tests (not auto-discovered
  by unittest discover since file doesn't match test*.py pattern)
- Add test-external-blueprints CI job to python-app.yml with continue-on-error: true
  (runs on single Python version only)
- Fix bayeswave.py: use setdefault() for quality dict to avoid KeyError when quality
  section doesn't exist in meta
- Fix analysis.py: calculate maximum frequency even when quality section is absent
  from event blueprint

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Update changelog and fix all tests to use local blueprints

- Add 'Unreleased' section to CHANGELOG.rst documenting the breaking change
  for minimum frequency location with migration guide and PR reference
- Update tests/blueprints.py helper module with absolute paths to local blueprint files
- Create tests/test_data/blueprints/production-pe-priors.yaml (local copy)
- Create tests/test_data/blueprints/gwtc-2-1/GW150914_095045.yaml (v0.7 compliant)
- Update test_dag.py, test_cli_manage.py, test_yaml.py, test_review.py, and
  tests/test_pipelines/test_bilby.py to use local blueprints via the blueprints module
  instead of external git.ligo.org URLs

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

* Fix test_build_submit_dryruns: add copy frames and osg to bayeswave defaults

The local production-pe.yaml was missing copy frames: True and osg: True in
the bayeswave scheduler section. These settings are present in the external
production-pe.yaml and are needed for the --copy-frames and --transfer-files
flags to appear in the BayesWave build_dag dryrun output, which
test_build_submit_dryruns asserts are present.

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

Author: Copilot

.github/workflows/python-app.yml

@@ -43,6 +43,39 @@ jobs:
       run: |
         python -m unittest discover tests/
 
+  test-external-blueprints:
+    # This job checks compatibility with the external asimov-data blueprints.
+    # It is allowed to fail because external data files may lag behind code changes.
+    runs-on: ubuntu-latest
+    continue-on-error: true
+    strategy:
+      fail-fast: false
+      matrix:
+        python-version: ["3.11"]
+
+    steps:
+    - uses: actions/checkout@v2
+      with:
+        fetch-depth: 0
+    - name: Set up Python ${{ matrix.python-version }}
+      uses: actions/setup-python@v2
+      with:
+        python-version: ${{ matrix.python-version }}
+    - name: Install dependencies
+      run: |
+        python -m pip install --upgrade pip
+        pip install .
+        pip install .[bilby]
+    - name: Set up git
+      run: |
+          git config --global init.defaultBranch master
+          git config --global user.email "you@example.com"
+          git config --global user.name "Your Name"
+
+    - name: Run external blueprint compatibility tests
+      run: |
+        python -m unittest tests.external_blueprint_compat -v
+
   publish:
     name: Build and publish to PyPI
     needs: build-ubuntu-python

CHANGELOG.rst

@@ -1,3 +1,41 @@
+Unreleased
+==========
+
+Breaking Changes
+----------------
+
+**Waveform Minimum Frequency Location**
+  The ``minimum frequency`` parameter **must** now be placed in the ``waveform``
+  section of a blueprint.  In earlier versions, placing it in the ``quality`` or
+  ``likelihood`` sections was permitted with a deprecation warning.  As of v0.7,
+  any blueprint that specifies ``minimum frequency`` under ``quality`` or
+  ``likelihood`` will be **rejected** with a ``ValueError`` and the blueprint
+  will not be applied.
+
+  To migrate an existing blueprint, move the value from the old location::
+
+      # Old (no longer valid in v0.7)
+      quality:
+        minimum frequency:
+          H1: 20
+          L1: 20
+
+  to the new location::
+
+      # Correct (v0.7+)
+      waveform:
+        minimum frequency:
+          H1: 20
+          L1: 20
+
+  The error message will indicate which section the value was found in and
+  prompt the user to update their blueprint accordingly.
+
+GitHub Pull Requests
+--------------------
+
++ `github#107 <https://github.com/etive-io/asimov/pull/107>`_: Enforce waveform minimum frequency location (breaking change for old blueprints)
+
 0.7.0-alpha2
 ============
 

asimov/analysis.py

@@ -1673,6 +1673,37 @@ def __init__(self, subject, name, pipeline, **kwargs):
         """
 
         self.category = config.get("general", "calibration_directory")
+        
+        # Early validation: Check for minimum frequency in wrong locations (v0.7)
+        # We need to check both the subject (event) metadata and the kwargs
+        # First, build the effective metadata as it will be in super().__init__
+        temp_meta = deepcopy(Analysis.meta_defaults)
+        
+        # Add pipeline defaults if available
+        if hasattr(subject, 'ledger') and subject.ledger and "pipelines" in subject.ledger.data:
+            if pipeline in subject.ledger.data["pipelines"]:
+                temp_meta = update(temp_meta, deepcopy(subject.ledger.data["pipelines"][pipeline]))
+        
+        # Add subject defaults
+        temp_meta = update(temp_meta, deepcopy(subject.meta))
+        
+        # Add kwargs
+        temp_meta = update(temp_meta, deepcopy(kwargs))
+        
+        # Now validate
+        if "quality" in temp_meta and "minimum frequency" in temp_meta["quality"]:
+            raise ValueError(
+                "Minimum frequency must be specified in the 'waveform' section, "
+                "not in the 'quality' section. Please update your blueprint to move "
+                "'minimum frequency' from 'quality' to 'waveform'."
+            )
+        if "likelihood" in temp_meta and "minimum frequency" in temp_meta["likelihood"]:
+            raise ValueError(
+                "Minimum frequency must be specified in the 'waveform' section, "
+                "not in the 'likelihood' section. Please update your blueprint to move "
+                "'minimum frequency' from 'likelihood' to 'waveform'."
+            )
+        
         super().__init__(subject, name, pipeline, **kwargs)
         self._checks()
 
@@ -1701,11 +1732,9 @@ def __init__(self, subject, name, pipeline, **kwargs):
             self.meta["sampler"]["lmax"] = self.meta["lmax"]
 
         # Check that the upper frequency is included, otherwise calculate it
-        if "quality" in self.meta:
-            if ("maximum frequency" not in self.meta["quality"]) and (
-                "sample rate" in self.meta["likelihood"]
-            ):
-                self.meta["quality"]["maximum frequency"] = {}
+        if "sample rate" in self.meta["likelihood"] and "interferometers" in self.meta:
+            if "maximum frequency" not in self.meta.get("quality", {}):
+                self.meta.setdefault("quality", {})["maximum frequency"] = {}
                 # Account for the PSD roll-off with the 0.875 factor
                 for ifo in self.meta["interferometers"]:
                     self.meta["quality"]["maximum frequency"][ifo] = int(

asimov/blueprints.py

@@ -89,6 +89,11 @@ class Waveform(Blueprint):
         description="The mode array to use in the waveform model.",
         default=None,
     )
+    minimum_frequency: dict[str, float] | None = pydantic.Field(
+        alias="minimum frequency",
+        description="The minimum frequency for the waveform model, given as a dictionary of values per interferometer.",
+        default=None,
+    )
 
 
     model_config = ConfigDict(extra='forbid')

asimov/configs/bilby.ini

@@ -15,6 +15,7 @@
 {%- assign priors = production.meta['priors'] -%}
 {%- assign data = production.meta['data'] -%}
 {%- assign quality = production.meta['quality'] -%}
+{%- assign waveform = production.meta['waveform'] -%}
 {%- assign ifos = production.meta['interferometers'] -%}
 
 {%- if data contains "calibration" %}
@@ -79,7 +80,7 @@ psd-length={{ likelihood['psd length'] | round }}
 psd-maximum-duration=1024
 psd-method=median
 psd-start-time=None
-minimum-frequency={ {% for ifo in ifos %}{{ifo}}:{{quality['minimum frequency'][ifo]}},{% endfor %}{% if likelihood contains 'start frequency'%} waveform: {{ likelihood['start frequency'] }} {% endif %} }
+minimum-frequency={ {% for ifo in ifos %}{{ifo}}:{{waveform['minimum frequency'][ifo]}},{% endfor %}{% if likelihood contains 'start frequency'%} waveform: {{ likelihood['start frequency'] }} {% endif %} }
 maximum-frequency={ {% for ifo in ifos %}{{ifo}}:{{quality['maximum frequency'][ifo]}},{% endfor %} }
 zero-noise=False
 tukey-roll-off={{ likelihood['roll off time'] | default: 0.4 }}

asimov/configs/lalinference.ini

@@ -11,6 +11,7 @@
 {%- assign priors = prior_interface.convert() -%}
 {%- assign data = production.meta['data'] -%}
 {%- assign quality = production.meta['quality'] -%}
+{%- assign waveform = production.meta['waveform'] -%}
 {%- assign ifos = production.meta['interferometers'] -%}
 {%- if production.event.repository -%}
 {%- assign repo_dir = production.event.repository.directory -%}
@@ -81,12 +82,12 @@ types = {'H1': '{{ production.meta['data']['frame types']['H1'] }}', 'L1': '{{ p
 channels = {'H1': '{{ production.meta['data']['channels']['H1'] }}', 'L1': '{{ production.meta['data']['channels']['L1'] }}', 'V1': '{{ production.meta['data']['channels']['V1'] }}'}
 
 [lalinference]
-flow = { {% if production.meta['interferometers'] contains "H1" %}'H1': {{ production.quality['minimum frequency']['H1'] }},{% endif %} {% if production.meta['interferometers'] contains "L1" %}'L1': {{ production.quality['minimum frequency']['L1']}},{% endif %} {% if production.meta['interferometers'] contains "V1" %} 'V1': {{ production.quality['minimum frequency']['V1']}} {% endif %} }
+flow = { {% if production.meta['interferometers'] contains "H1" %}'H1': {{ waveform['minimum frequency']['H1'] }},{% endif %} {% if production.meta['interferometers'] contains "L1" %}'L1': {{ waveform['minimum frequency']['L1']}},{% endif %} {% if production.meta['interferometers'] contains "V1" %} 'V1': {{ waveform['minimum frequency']['V1']}} {% endif %} }
 fhigh = { {% if production.meta['interferometers'] contains "H1" %}'H1': {{ production.meta['quality']['high frequency'] }},{% endif %} {% if production.meta['interferometers'] contains "L1" %}'L1': {{ production.meta['quality']['high frequency'] }},{% endif %} {% if production.meta['interferometers'] contains "V1" %} 'V1': {{ production.meta['quality']['high frequency'] }} {% endif %} }
 
 [engine]
 
-fref={{ production.meta['waveform']['reference frequency'] }}
+fref={{ waveform['reference frequency'] }}
 
 approx = {{ production.meta['waveform']['approximant'] }}
 amporder = {{ prior_interface.get_amp_order() }}

asimov/configs/rift.ini

@@ -62,7 +62,7 @@ types = { {% for ifo in ifos %}"{{ifo}}":"{{data['frame types'][ifo]}}",{% endfo
 channels = { {% for ifo in ifos %}"{{ifo}}":"{{data['channels'][ifo]}}",{% endfor %} }
 
 [lalinference]
-flow = { {% for ifo in ifos %}"{{ifo}}":{{quality['minimum frequency'][ifo]}},{% endfor %} }
+flow = { {% for ifo in ifos %}"{{ifo}}":{{waveform['minimum frequency'][ifo]}},{% endfor %} }
 fhigh = { {% for ifo in ifos %}"{{ifo}}":{{quality['maximum frequency'][ifo]}},{% endfor %} }
 
 [engine]

asimov/pipeline.py

@@ -522,6 +522,15 @@ def submit_dag(self, dryrun=False):
         configfile = self.production.event.repository.find_prods(
             self.production.name, self.category
         )[0]
+        
+        # Validate minimum frequency format
+        min_freq = self.production.meta["waveform"]["minimum frequency"]
+        if not isinstance(min_freq, dict) or not min_freq:
+            raise ValueError(
+                "Minimum frequency in 'waveform' section must be a non-empty dictionary "
+                "mapping interferometer names to frequency values."
+            )
+        
         command = [
             "--webdir",
             os.path.join(
@@ -537,7 +546,7 @@ def submit_dag(self, dryrun=False):
             "--approximant",
             self.production.meta["waveform"]["approximant"],
             "--f_low",
-            str(min(self.production.meta["quality"]["minimum frequency"].values())),
+            str(min(min_freq.values())),
             "--f_ref",
             str(self.production.meta["waveform"]["reference frequency"]),
         ]

asimov/pipelines/bayeswave.py

@@ -51,7 +51,7 @@ def __init__(self, production, category=None):
             self.logger.info("Assuming analyses directory.")
 
         if not production.meta.get("quality", {}).get("lowest minimum frequency", None):
-            production.meta["quality"]["lowest minimum frequency"] = self.flow
+            production.meta.setdefault("quality", {})["lowest minimum frequency"] = self.flow
 
     def build_dag(self, user=None, dryrun=False):
         """
@@ -288,7 +288,20 @@ def flow(self):
         minimum frequency from the list of interferometer
         lower frequencies.
         """
-        return min(self.production.meta["quality"]["minimum frequency"].values())
+        if "waveform" not in self.production.meta or "minimum frequency" not in self.production.meta["waveform"]:
+            raise ValueError(
+                "Minimum frequency must be specified in the 'waveform' section. "
+                "Please update your blueprint to include 'minimum frequency' in 'waveform'."
+            )
+        
+        min_freq = self.production.meta["waveform"]["minimum frequency"]
+        if not isinstance(min_freq, dict) or not min_freq:
+            raise ValueError(
+                "Minimum frequency in 'waveform' section must be a non-empty dictionary "
+                "mapping interferometer names to frequency values."
+            )
+        
+        return min(min_freq.values())
 
     def before_submit(self):
         """

asimov/pipelines/pesummary.py

@@ -268,9 +268,15 @@ def submit_dag(self, dryrun=False):
                         if "reference frequency" in dep_analysis.meta["waveform"]:
                             f_refs.append(str(dep_analysis.meta["waveform"]["reference frequency"]))
 
-                    if "quality" in dep_analysis.meta:
-                        if "minimum frequency" in dep_analysis.meta["quality"]:
-                            f_lows.append(str(min(dep_analysis.meta["quality"]["minimum frequency"].values())))
+                    if "waveform" in dep_analysis.meta:
+                        if "minimum frequency" in dep_analysis.meta["waveform"]:
+                            min_freq = dep_analysis.meta["waveform"]["minimum frequency"]
+                            if isinstance(min_freq, dict) and min_freq:
+                                f_lows.append(str(min(min_freq.values())))
+                            else:
+                                self.logger.warning(
+                                    f"Invalid minimum frequency format in {dep_analysis.name}, skipping"
+                                )
 
                     # Config file should be added for each analysis that has samples
                     if dep_config:
@@ -359,11 +365,18 @@ def submit_dag(self, dryrun=False):
             # If we have per-analysis f_low values, use them
             command += ["--f_low"]
             command.extend(f_lows)
-        elif "minimum frequency" in quality_meta:
-            command += [
-                "--f_low",
-                str(min(quality_meta["minimum frequency"].values())),
-            ]
+        elif "minimum frequency" in waveform_meta:
+            min_freq = waveform_meta["minimum frequency"]
+            if isinstance(min_freq, dict) and min_freq:
+                command += [
+                    "--f_low",
+                    str(min(min_freq.values())),
+                ]
+            else:
+                raise ValueError(
+                    "Minimum frequency in 'waveform' section must be a non-empty dictionary "
+                    "mapping interferometer names to frequency values."
+                )
 
         # f_ref - use per-analysis values if available, otherwise use global
         if is_subject_analysis and f_refs: