Fix: engine-test emissions computed as zero + follow-up UX/docs cleanups - #349
Merged
Conversation
Reported after real QGIS testing: after loading engine test events via
the per-source dialog and regenerating the emission inventory, the
Results Analysis showed zero emissions for TESTPAD_A EngineTestSource
even though the events were correctly written to the DB and the
underlying compute path (Emission.add, getEmissionIndexByMode) works
correctly in isolation.
Root cause
----------
EmissionCalculation.__init__ only calls Singleton.reset_all() when the
db_path CHANGES between runs. Rationale in the code:
# Flush all Singleton stores only when the user opens a *different*
# .alaqs database in the same QGIS Python session. Resetting on
# every calculation is unnecessary: the stores are still valid for
# the same DB path.
But writes to the DB via a sibling channel (like this CSV import
dialog) BREAK that assumption. If the user has triggered any emission
calculation earlier in the same QGIS session — for any reason — then
EngineTestEventsStore is a Singleton already holding the events state
AS OF THAT RUN (typically empty). Loading events via the dialog writes
directly to the DB via sqlite3, bypassing the Singleton. The next
emission calc run sees the same db_path and skips reset_all, so the
cached (empty) events store keeps serving its stale contents.
EngineTestSourceModule.process() iterates the cached zero events,
produces zero emissions.
Fix
---
After a successful apply_insert, reset the two events-related
Singletons that could be caching pre-import state:
* EngineTestEventsStore (Store keyed by db_path; holds Event objects)
* EngineTestEventsDatabase (SQLSerializable; holds the raw dict)
The reset is a best-effort cache invalidation. Wrapped in try/except
so cache-management errors don't fail the user's apply. Other
Singletons (AircraftStore, EngineStore, AreaSourcesStore,
AmbientConditionStore) are not touched because an engine-test-events
CSV import cannot affect those tables.
Not affected by this pattern
----------------------------
The area-source form's is_test_site toggle IS mediated through the
QGIS attribute layer's writeAttribute path, which the plugin picks
up on layer save; that path doesn't go through raw sqlite3, so no
similar staleness applies.
Tests
-----
No new unit tests. The bug is a race between Singleton lifecycle and
raw DB writes, both mediated by QGIS session state and neither
observable outside a real QGIS run. Manual verification: load
events, regenerate inventory, check Results Analysis for TESTPAD_A
EngineTestSource NOx on 2025-12-01T09:00:00 → should be non-zero
(previously was zero because the events store was cached empty).
Reported after real QGIS testing: TESTPAD_A (a test-site area source) appeared in the Results Analysis "Emission Source Name" dropdown under BOTH "AreaSource" and "EngineTestSource". Selecting it under "AreaSource" silently produced zero emissions (process() at line 84 skips test sites), which is confusing. Root cause ---------- AreaSourceModule and EngineTestSourceModule both use AreaSourcesStore as their backing store. The base SourceModule.loadSources() (SourceModule.py:118) iterates the entire store and populates _sources unconditionally. Results Analysis reads _sources for the dropdown, so both modules list every area source. The two modules' process() methods DO filter (AreaSource skips is_test_site='1'; EngineTest skips others), so compute is double-counting-free — but the dropdown UI still shows the misleading duplicates. Fix --- Override loadSources() in each module to apply the same filter its process() uses: * AreaSourceModule.loadSources: skip sources where isTestSite() is True. Now the "AreaSource" dropdown lists only regular area sources. * EngineTestSourceModule.loadSources: keep only sources where isTestSite() is True. Now the "EngineTestSource" dropdown lists only test sites. Each area source appears under exactly one module dropdown, determined by its is_test_site flag. The process() methods retain their own filters as defensive belt-and-braces — cheap in-loop check that guards against future callers of getSources() from any other codepath. Tests ----- No new unit tests. The behavior is a UI listing detail; testing it meaningfully requires a QGIS session. Existing 71 QGIS-free tests still pass.
Reported after real QGIS testing: even with the previous fix that
skipped validation of the *_kg_unit fields for engine-test sites, the
fields were still visually active — users could type numbers into
them without any visual cue that those values would be ignored at
compute time.
Change
------
Add ``_apply_test_site_field_state(fields, is_test_site)`` helper.
When ``is_test_site=True`` it disables (grays out):
* Units per Year (``unit_year``)
* All seven emission-rate line edits (``co_kg_unit``, ``hc_kg_unit``,
``nox_kg_unit``, ``sox_kg_unit``, ``pm10_kg_unit``, ``p1_kg_unit``,
``p2_kg_unit``)
* The three profile combo boxes (``hourly_profile``,
``daily_profile``, ``monthly_profile``)
When ``is_test_site=False`` it re-enables them, letting users switch
a source back to a regular area source configuration.
Called at form-open with the seeded checkbox state, and connected to
the checkbox's ``toggled`` signal so the visual state tracks live
edits.
Fields that STAY enabled regardless
-----------------------------------
* Source Name — the source identifier
* Height — matters for dispersion positioning regardless of source
type
Fields that stay disabled regardless
------------------------------------
* Heat Flux — already 0 and disabled for all area sources
Compute-side rationale
----------------------
EngineTestSourceModule.process() reads events from engine_test_events
and ignores every one of the disabled fields. AreaSourceModule
already skips test sites (line 84), so disabled fields never
contribute anyway. Disabling is a UI cue, not a functional change.
Tests
-----
No new unit tests. The behavior is Qt-widget wiring against a
QCheckBox and eleven other widgets; testing meaningfully requires a
QGIS session. Manual verification: tick "Engine test site" — the
Emissions tab fields, Units per Year, and Profiles combo boxes go
gray. Untick — they come back.
Discovered during real-QGIS testing of the plugin side: the standalone
twin ``openalaqs_standalone/compute_engine_test.py`` was looking up
``co_ei_g_kg_fuel``, ``nox_ei_g_kg_fuel``, ``hc_ei_g_kg_fuel`` etc. in
the ``ei_lookup`` dict — but the actual DB column names in
``default_aircraft_engine_ei`` are ``co_ei``, ``nox_ei``, ``hc_ei``
(no ``_g_kg_fuel`` suffix). Every pollutant lookup returned None, so
every event contributed only fuel; all pollutant totals stayed at
zero.
The bug never surfaced before because:
* The standalone unit tests used a fixture (``_ei`` helper) that
ALSO built dicts with the wrong ``co_ei_g_kg_fuel`` keys, so
the same wrong lookups matched. Tests passed vacuously.
* The standalone twin isn't exercised in production yet — plugin
users go through the QGIS compute path, which uses a different
key mapping via ``EngineEmissionIndex.setObject`` (Engine.py:755)
that correctly maps DB ``co_ei`` -> internal ``co_g_kg``.
Verification: standalone compute against the user's EHRD .alaqs now
produces NOx = 1438.92 g for period 2025-12-01 09:00-10:00, matching
the hand calculation (event LOG-2025-001: C25C, 2x TFE731-2-2B,
TX 600s + CL 300s):
* TX: 0.024 kg/s * 1200 engine-s = 28.8 kg fuel; 2.82 * 28.8 = 81.2 g NOx
* CL: 0.173 kg/s * 600 engine-s = 103.8 kg fuel; 13.08 * 103.8 = 1357.7 g NOx
* Total: 132.6 kg fuel, 1438.9 g NOx
Files
-----
* openalaqs_standalone/compute_engine_test.py
- ``_add_ei_to_totals``: key map now uses ``co_ei``, ``hc_ei``,
``nox_ei``, ``sox_ei``, ``pm10_ei``, ``p1_ei``, ``p2_ei`` (and
bare ``pm10_nonvol``, ``pm10_sul``, ``pm10_organic`` for PM
subclasses, matching the DB columns).
- ``_build_icao_eedb_for_engine``: BFFM2 pollutant map now uses
``nox_ei``, ``co_ei``, ``hc_ei``.
- BFFM2 ``_EEDB_PASSTHROUGH``: PM/SOx column names corrected.
- CO2 no longer looked up from a column (there is none); computed
from fuel_burned * 3160 g/kg, matching the plugin's
Emissions.py:defaultEI value.
- Docstring updated to describe the actual ei_row schema.
* openalaqs_standalone/validation/tests/test_phase3_engine_test_standalone.py
- ``_ei`` helper now builds dicts with the correct DB column names.
PM subclasses (``pm10_nonvol``, etc.) use bare names; other
pollutants get ``_ei`` suffix.
- The 4 BFFM2 tests and the hand-calc test now exercise the real
column shape.
All 26 tests in the phase-3 engine test file pass.
Full standalone regression: 976 passed.
Reflects the four fixes shipped in this branch: the CSV import
dialog is now on the area-source form (not the toolbar), rows for
other sources are silently skipped, form fields gray out on
test-site ticks, the Results Analysis dropdown filters by
is_test_site, and the events store cache is invalidated after
import.
Changes
-------
* documents/USER_GUIDE.md, step 5 in the "Engine test sites"
section: rewrite steps 2-5. Checkbox now grays fields (not just
ignores their values); source_id is optional in the CSV; master
CSV with silent skip is explained; the button on the area-source
form is the primary path (toolbar action removed). CLI is
positioned as the scripting / multi-source bulk-load path with
the extra replace-all mode.
* README.md, engine-test-runs paragraph: swap toolbar action for
the source-form button. CLI still mentioned as the scripting
path.
* CHANGELOG.md: rewrite the "CSV import tool" bullet to describe
the per-source dialog (primary) and CLI (scripting + bulk-load).
Add three new bullets:
- Form UX for test sites (gray-out behavior)
- Source-name dropdown filter in Results Analysis
- Cache invalidation after CSV import
The is_test_site KeyError guard bullet is preserved as-is (still
accurate; pre-v1b studies still need the migration).
Not changed
-----------
* scripts/README.md — the CLI documentation was already accurate.
* documents/USER_GUIDE.md sections unrelated to engine-test sites.
Add `training_engine_test_events.csv` and a short README documenting
what it demonstrates and how to load it. Sample includes 5 events on
a single test-pad source (TESTPAD_A):
* Rows 1, 2, 5: implicit engine_count (falls back to
default_aircraft.engine_count for the aircraft type).
* Rows 3, 4: explicit engine_count override (2 and 1
respectively).
* All rows: implicit engine_uid (falls back to
default_aircraft.engine for the aircraft type).
* Two aircraft types (C25C, PC24), all four LTO modes exercised
across the events.
Dates in 2025 to match a typical modern study's inventory period.
The README covers the load workflow, adapting to a different year,
and links back to USER_GUIDE.md §Engine test sites for the full
CSV specification.
Validated against read_csv / validate_csv_rows: 5 valid rows,
0 errors, 0 warnings.
Companion to the docs commit that updated USER_GUIDE, README, and
CHANGELOG for the per-source dialog workflow.
…d no visual effect)
Reported after real QGIS testing: only Units per Year went gray when
"Engine test site" was ticked; the Profiles combo boxes and the
Emissions tab's *_kg_unit line edits stayed fully active despite
form.findChild finding the widgets and setEnabled(False) being
called on them.
Diagnosis
---------
Per-widget setEnabled works correctly for widgets on the
Parameters tab (index 0, always-visible by default). unit_year got
grayed as intended; heat_flux (already disabled with '0' by earlier
code) stayed disabled.
Widgets nested inside the Profiles tab (index 1) and Emissions tab
(index 2) of the QTabWidget don't respond to per-widget setEnabled
calls the way tab-0 widgets do — QGIS' attribute form seems to
override enabled state on tab widgets when the tab is switched
into, so the disable doesn't stick or isn't rendered.
Fix
---
Disable at the tab level instead. QTabWidget.setTabEnabled(index,
False) grays out the tab label AND makes it unclickable, which
signals "this tab's contents don't apply" more clearly than
graying individual fields would anyway.
* Parameters tab (index 0): keep per-widget disable on unit_field
(it's the only irrelevant field on this tab; height and
heat_flux stay as they were).
* Profiles tab (index 1): setTabEnabled(1, False) when checkbox
is ticked.
* Emissions tab (index 2): setTabEnabled(2, False) when checkbox
is ticked.
Untick re-enables both tabs.
Files
-----
* open_alaqs/ui/ui_area_sources.py
- form_open: add `tab_widget=form.findChild(QtWidgets.QTabWidget,
"tabWidget")` to the fields dict.
- _apply_test_site_field_state: simplified. Direct setEnabled on
unit_field (Parameters tab); setTabEnabled on tab_widget for
the Profiles and Emissions tabs.
Not touched: on_save still auto-fills "0" into the *_kg_unit fields
and unit_year when the source is a test site — even though users
can't reach those fields when the Emissions/Profiles tabs are
disabled, the underlying QLineEdit values still get written to the
feature, and the auto-fill guarantees valid DB state.
Manual verification: create area source, tick "Engine test site" —
Units per Year grays out, the Profiles and Emissions tab labels
gray out and become unclickable. Untick — all three come back.
Reported: the section adds noise more than value. Users who need to adapt dates can figure it out on their own; the specific SQL command was misleading anyway (it hardcoded the source_id and 2026 target).
…EI store
Root cause of "engine-test emissions show zero" reported after all
prior fixes (Singleton reset, source filter, form UX) had landed and
QGIS had been restarted.
Diagnosis
---------
Real-QGIS diagnostic script confirmed every step of the compute up to
_resolve_ei works correctly:
[1] Events load from DB (5 events, all instudy=True)
[2] Correct event overlaps target period
[3] AircraftStore has C25C -> default engine '1AS001'
[4] EngineStore has 1AS001, EI values correct for all four modes
[5] EngineTestSourceModule loads TESTPAD_A after beginJob
[6] process(period) returns 0 entries <-- FAILURE
The failure is inside _add_event_to_emission via _resolve_ei.
The mode-lookup methods (getEmissionIndexByMode,
getEmissionIndexByModeWithMEEM, getEmissionIndexByModeWithBFFM2) live
on the EngineEmissionIndex STORE returned by
engine.getEmissionIndex(), NOT on the Engine object itself. The real
open_alaqs.core.interfaces.Engine.Engine class has NO
getEmissionIndexByMode method.
_resolve_ei was calling engine.getEmissionIndexByMode(mode) directly,
which raises AttributeError. The try/except at the end silently
swallowed it and returned None. Every mode returned None -> every
mode was skipped -> any_contribution stayed False -> emission was
NOT appended to result -> process() returned an empty list ->
zero emissions in the output.
MovementEmissionCalculator uses the correct two-level pattern:
self._engine.getEmissionIndex().getEmissionIndexByModeWithMEEM(...)
Fix
---
open_alaqs/core/modules/EngineTestSourceModule.py:
* _resolve_ei extracts ei_store = engine.getEmissionIndex() once at
the top, then calls the three mode-lookup methods on that store
instead of on the engine.
* All three thrust modes updated: snap, meem, bffm2.
* Guard against engine having no EI store (returns None).
* Docstring gains an implementation-note paragraph flagging the
correct two-level API to prevent this regression.
Why did the tests miss it
-------------------------
tests/test_phase3_engine_test_source_module_plugin.py used FakeEngine
classes with the mode-lookup methods flat on the top-level fake. The
tests validated the API path I INTENDED, not the API path Engine
actually exposes. Since the tests are QGIS-guarded (skipped in the
sandbox), CI vacuously passed too.
Test fixes
----------
* Added _wrap_as_engine(ei_store) helper that wraps an
EI-store-shaped fake in an object with getEmissionIndex(),
matching the real Engine API.
* Updated four inline _resolve_ei call sites (Section 2) to wrap
their FakeEngines.
* _FakeEngine (used by _make_module_with_fakes) rewritten to
expose the two-level shape internally; keeps the same
per-mode-EI constructor API for backward compatibility with
existing test code.
* _FakeEngineStore.getObject() auto-wraps engines that don't
already have getEmissionIndex, so inline test-local classes
like DualEngine and BFFM2Engine can keep their flat shape for
readability while feeding the module the correct two-level
shape.
The fix and the test rewrite are shipped together so future runs
against real QGIS exercise the actual API surface.
sstromat
pushed a commit
that referenced
this pull request
Aug 4, 2026
Closes the two-part gap that meant engine test sites reached the QGIS compute path (fixed in PR #349) but never showed up in either the standalone parquet outputs or the plugin's AUSTAL export. Standalone (openalaqs_standalone) ---------------------------------- extract_sources.py: emit test sites as source_type='engine_test' with source_id 'engine_test:<oid>' instead of skipping them. Regular area sources still come through as source_type='area', source_id 'area:<oid>'. is_test_site is passed through in extra_json for downstream consumers. Docstring updated to list all 6 source types. compute_area.py: filter out test sites in the WHERE clause so their emissions don't get double-counted between the area path (0 anyway, since test sites have no *_kg_unit rates) and the new engine-test path. PRAGMA table_info probe guards against pre-v1b DBs that lack the is_test_site column entirely. compute_engine_test.py: new compute_engine_test_emissions() wrapper matching the standard (alaqs_path, year, pollutants, time_window) signature. Iterates only hours where events actually overlap the window (cheaper than looping 8760 empty hours), calls the existing compute_engine_test_for_period per hour, emits long-form (timestamp, source_id, pollutant, kg_in_hour) rows in kg. Internal pollutant 'p2' is mapped to standard 'pm25' on output. New helpers _load_ei_lookup and _load_aircraft_lookup read the SQL tables the per-period compute needs. orchestrate.py: append (compute_engine_test_emissions, 'engine_test') to stationary_computes so studies with test sites transparently produce engine_test rows in emissions.parquet. Always-on (no flag needed): a study with zero events contributes zero rows. Plugin AUSTAL (open_alaqs/core/modules/AUSTALOutputModule.py) ------------------------------------------------------------ _ti_type_label_for: distinguish engine-test AreaSources instances (isTestSite() == True) from regular area sources by returning 'engine_test' instead of 'area'. The two share the AreaSources Python class (test sites are rows in shapes_area_sources with is_test_site='1'), so class-name-only dispatch would merge their per-hour g/s rates with much larger, constant regular-area rates in the by-type aggregation. Merging would smear the sparse test-event spikes into the ambient area emission and both dispersion signatures would be lost. Bare AreaSources subclasses without isTestSite() fall back to 'area' unchanged; a broken accessor that raises is caught defensively. The rest of the AUSTAL export flow needs no changes: emissions flow through the standard (source, [emission]) tuple path already, and downstream aggregation / grid file writing dispatches on group id strings (not hard-coded to any particular type name). Dataiku recipes --------------- standalone_recipe.txt and austal_prep_recipe.txt need NO changes: the standalone recipe calls orchestrate() with generic kwargs, which picks up engine_test via stationary_computes; the austal_prep recipe calls run_austal_prep() and austal_prep dispatches on geometry_kind (polygon), not source_type, so engine_test sources are handled identically to regular area sources. Verification (against user's EHRD DB with TESTPAD_A + 5 events) --------------------------------------------------------------- * Standalone compute_engine_test_emissions: full-year run produces 30 rows (5 events × 6 pollutants), Jan 15 window produces 12 rows. Values match hand calc and QGIS plugin output (1.898 kg CO at 09:00, 1.439 kg NOx at 09:00). * Full orchestrate for Jan 15: sources.parquet: 3606 sources ({'road': 3588, 'gate': 10, 'parking': 7, 'engine_test': 1}) emissions.parquet: 495,372 rows (engine_test: 12) * austal_prep on those outputs succeeded: TESTPAD_A appeared as slots 01/02 in series.dmna with hour 10:00 CO=5.273e-01 g/s (Jan 15 09:00 event) and hour 11:00 CO=8.531e-01 g/s (Jan 15 10:00 event). AUSTAL uses hour-ending convention. Tests ----- * Rewrote 2 tests in test_phase1b_extract_sources_standalone.py that asserted the old skip-and-log behavior for test sites (now assert they're emitted as source_type='engine_test'). Simplified a third test. * New test_austal_engine_test_label.py covers _ti_type_label_for for regular area, engine-test area, roadway, old AreaSources without isTestSite, broken isTestSite, and unknown classes. Requires real QGIS (uses pytest.importorskip); skips cleanly in developer sandboxes without QGIS, same pattern as test_austal_slot_compaction.py. * Full standalone suite: 1,042 passed, 1 skipped (test_austal requires QGIS).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the earlier post-test-site UX fixes. Three commits on top
of
75e061b.The main fix (
a92c00f)Root cause of "engine-test emissions show zero" reported after every
prior fix (Singleton reset, source filter, form UX) had landed and
QGIS had been restarted.
Real-QGIS diagnostic confirmed every step of the compute up to
_resolve_eiworks correctly: events load, aircraft resolves, engineresolves, EI values are correct for all four modes. But
process()returns 0 entries.
_resolve_eiwas callingengine.getEmissionIndexByMode(mode)directly on the
Engineobject. But the mode-lookup methods(
getEmissionIndexByMode,getEmissionIndexByModeWithMEEM,getEmissionIndexByModeWithBFFM2) live on theEngineEmissionIndexSTORE returned by
engine.getEmissionIndex(), NOT on the Engineitself. The real
Engineclass has nogetEmissionIndexByModemethod.
The
AttributeErrorwas silently swallowed by thetry/exceptatthe end of
_resolve_ei, which returnedNone. Every mode returnedNone → every mode was skipped →
any_contributionstayed False →emission was not appended →
process()returned an empty list →zero emissions in the output.
MovementEmissionCalculatoruses the correct two-level pattern:Fix in
_resolve_ei: extractei_store = engine.getEmissionIndex()once at the top, then call the three mode-lookup methods on that
store instead of on the engine. Docstring gains an implementation
note flagging the correct two-level API to prevent this regression.
Why the tests missed it
tests/test_phase3_engine_test_source_module_plugin.pyusedFakeEngineclasses with the mode-lookup methods flat on thetop-level fake. The tests validated the API I intended, not the API
Engineactually exposes. Since the tests are QGIS-guarded (skippedin the sandbox), CI vacuously passed too.
Test fixes shipped alongside:
_wrap_as_engine(ei_store)helper wraps an EI-store-shapedfake in an object with
getEmissionIndex(), matching the realEngineAPI._resolve_eicall sites in Section 2 updated to wraptheir
FakeEngines._FakeEngine(used by_make_module_with_fakes) rewritten toexpose the two-level shape internally; keeps the same
per-mode-EI constructor API for backward compatibility.
_FakeEngineStore.getObject()auto-wraps engines that don'talready have
getEmissionIndex, so inline test-local classes likeDualEngineandBFFM2Enginecan keep their flat shape forreadability while feeding the module the correct two-level shape.
Secondary fix (
4eeb5bb)Per-widget
setEnabled(False)on the*_kg_unitline edits andProfile combo boxes had no visual effect — QGIS' attribute form seems
to override enabled state on widgets nested inside
QTabWidgettabs.Replaced per-widget disable with
QTabWidget.setTabEnabled(index, False)for the Profiles (index 1) and Emissions (index 2) tabs. Thetab label grays out AND becomes unclickable, which signals "this
tab's contents don't apply" more clearly than graying individual
fields would.
unit_yearon the Parameters tab (index 0) keeps itsper-widget disable — that worked correctly.
Docs cleanup (
2bd2679)Removed the "Adapting to a different year" section from
example/training/training_engine_test_events.README.md. Adds noisemore than value; users who need to adapt dates can figure it out on
their own.
Tests
two-level Engine API; will pass in CI's QGIS environment.
Verification
Manual: restart QGIS, reopen study with an engine-test site and
loaded events, regenerate the emission inventory. TESTPAD_A
EngineTestSource emissions at any event's overlapping hour should now
show non-zero. For the sample CSV in
example/training/, that means2025-01-15 09:00, 2025-01-15 10:00, 2025-01-16 08:00, 2025-02-03
11:00, and 2025-02-03 14:00.