Rename "occupancy_helper" for consistency and clarity #3901
Conversation
WalkthroughRenames the occupancy entry point and public name from occupancy_helper → occupancy, moves the main function to cea.demand.occupancy.occupancy_main, and shifts configuration access from config.occupancy_helper to config.occupancy. Updates schemas, scripts, tests, workflows, and docstrings to the new name; no logic or schema changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Workflow / Script Runner
participant Occupancy as cea.demand.occupancy
participant Config as Configuration
participant FS as Filesystem (inputs/outputs)
Note over Runner,Occupancy #EDF7FF: Invocation renamed from "occupancy-helper" → "occupancy"
Runner->>Occupancy: call occupancy_main(locator, config, building?)
Occupancy->>Config: read config.occupancy.buildings, config.occupancy.occupancy_model
Occupancy->>FS: read inputs (building_internal, building_comfort, building_architecture, zone_geometry, weather_file...)
Occupancy-->>FS: write occupancy outputs (schedules / CSVs)
Occupancy-->>Runner: return status / logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cea/scripts.yml (1)
176-188: CLI parameters still point to occupancy-helper; this will bypass config.occupancy values at runtimeThe script now reads from
config.occupancy.*, but the CLI wiring exposesoccupancy-helperparameters. This mismatch will cause CLI/Dashboard changes to be written under[occupancy-helper]and ignored by the code. Update the parameters group tooccupancyto align with the code.- name: occupancy-helper label: "Energy Demand Part 1: building occupancy" short_description: Estimate building occupancy profiles using CEA models description: This Feature uses CEA models and input schedules to estimate the occupancy profile of buildings. interfaces: [dashboard, cli] - module: cea.demand.occupancy + module: cea.demand.occupancy parameters: ['general:scenario', 'general:multiprocessing', 'general:number-of-cpus-to-keep-free', 'general:debug', - occupancy-helper] + occupancy]If you intend to keep the CLI name
occupancy-helperfor backward compatibility, that’s fine—just ensure the parameter section isoccupancy. I can open a follow-up PR to add a deprecation note in docs.cea/tests/test_schedules.py (1)
135-139: Bug: occupancy_main returns None; create_data breaks when reading schedules from the return value
occupancy_mainwrites files and returns None (see occupancy.py). The current code assigns its return tocalculated_schedulesand then indexes it, which will raise at runtime.Fix by generating the schedules and then reading the produced CSV:
- calculated_schedules = occupancy_main(locator, config) - if not test_config.has_section('test_mixed_use_schedules'): - test_config.add_section('test_mixed_use_schedules') - test_config.set('test_mixed_use_schedules', 'reference_results', json.dumps( - {schedule: calculated_schedules[schedule][REFERENCE_TIME] for schedule in calculated_schedules.keys()})) + # Generate schedules for B1011 and read the resulting CSV + occupancy_main(locator, config, building='B1011') + calculated_schedules = pd.read_csv(locator.get_occupancy_model_file('B1011')).set_index('date') + if not test_config.has_section('test_mixed_use_schedules'): + test_config.add_section('test_mixed_use_schedules') + test_config.set( + 'test_mixed_use_schedules', + 'reference_results', + json.dumps({schedule: calculated_schedules[schedule][REFERENCE_TIME] + for schedule in calculated_schedules.columns}) + )
🧹 Nitpick comments (5)
cea/demand/occupancy.py (1)
480-483: Tidy the log message (double space) and prefer f-stringsMinor UX nit: fix the extra space and modernize the formatting.
- print('Running occupancy model with schedule model=%s' % config.occupancy.occupancy_model) + print(f'Running occupancy model with schedule model={config.occupancy.occupancy_model}')cea/schemas.yml (1)
1769-1769: Back-compat consideration: temporary dual listing.If any external tooling parses used_by to resolve script names, consider (optionally) listing both “occupancy” and “occupancy_helper” for one release cycle, then remove the latter.
Example (pattern only):- used_by: - - occupancy + used_by: + - occupancy + - occupancy_helper # deprecated; remove next releaseAlso applies to: 1824-1824, 1918-1918, 2113-2114, 2409-2409, 2458-2458, 2529-2529, 2560-2560, 2591-2591, 2661-2661, 2733-2733, 10571-10572, 19442-19443, 21040-21041, 21174-21175
cea/tests/test_schedules.py (1)
56-56: Scope the run to a single building and force determinism for stability/perfLimit schedule generation to B1011 (the only file you read) and explicitly set the deterministic model to avoid stochastic drift and speed up the test.
Apply:
- occupancy_main(locator, config) + # Ensure deterministic schedules and limit computation to the needed building + config.occupancy.occupancy_model = "deterministic" + occupancy_main(locator, config, building='B1011')If
config.multiprocessingwas previously used, confirmconfig.general.multiprocessing = Falseis the current way to disable parallelism for tests.cea/tests/create_unittest_data.py (2)
56-56: Make fixture generation reproducible and fasterSet deterministic occupancy and disable multiprocessing before generating schedules for B1011.
- occupancy_main(locator, config, building='B1011') + # Make fixtures reproducible and fast + config.general.multiprocessing = False + config.occupancy.occupancy_model = "deterministic" + occupancy_main(locator, config, building='B1011')
129-129: Consistency with config API when disabling parallelismElsewhere we use
config.general.multiprocessing = False. Keep it consistent here to ensureget_number_of_processes()behaves as expected.Update in
run_for_single_building(context outside the changed line):# before config.multiprocessing = False # after config.general.multiprocessing = FalseIf unsure, search the repo for
get_number_of_processesand confirm it reads fromconfig.general.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cea/demand/occupancy.py(2 hunks)cea/schemas.yml(16 hunks)cea/scripts.yml(1 hunks)cea/tests/create_unittest_data.py(3 hunks)cea/tests/test_calc_thermal_loads.py(3 hunks)cea/tests/test_schedules.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
cea/tests/test_schedules.py (1)
cea/demand/occupancy.py (1)
occupancy_main(35-92)
cea/demand/occupancy.py (3)
cea/inputlocator.py (1)
InputLocator(20-1474)cea/config.py (1)
Configuration(35-277)cea/datamanagement/void_deck_migrator.py (1)
migrate_void_deck_data(7-35)
cea/tests/create_unittest_data.py (1)
cea/demand/occupancy.py (1)
occupancy_main(35-92)
cea/tests/test_calc_thermal_loads.py (1)
cea/demand/occupancy.py (1)
occupancy_main(35-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests (macos-latest)
- GitHub Check: tests (windows-latest)
🔇 Additional comments (9)
cea/schemas.yml (4)
1769-1769: Rename applied consistently to building property schemas.used_by now lists "occupancy" for get_building_architecture, get_building_comfort, get_building_internal, and get_building_weekly_schedules. Looks correct.
Also applies to: 1824-1824, 1918-1918, 2113-2114
2409-2409: Envelope database dependencies updated to “occupancy”.Good alignment with the module rename; no schema keys changed.
Also applies to: 2458-2458, 2529-2529, 2560-2560, 2591-2591, 2661-2661, 2733-2733
10571-10572: Radiation/envelope systems used_by includes “occupancy”. Verify actual usage.If occupancy doesn’t read envelope/radiation-derived systems directly, consider dropping redundant used_by entries to keep dependency graphs tight.
Would you like a quick grep script to confirm references in cea/demand/occupancy.py?
19164-19165: Everything now uses outputs/data/occupancy via get_occupancy_model_file; no occupancy_helper references remain.cea/tests/test_schedules.py (1)
17-17: Import rename LGTMImport updated to the new API is correct and consistent with the PR intent.
cea/tests/create_unittest_data.py (1)
21-21: Import rename LGTMImport aligns with the new module/function naming.
cea/tests/test_calc_thermal_loads.py (3)
14-14: Import rename LGTMCorrect import of the new entry point.
66-67: Deterministic occupancy and new API usage LGTMExplicitly setting
config.occupancy.occupancy_modeland callingoccupancy_mainwith the building is correct and keeps the test stable.
122-122: Per-building occupancy generation LGTMCalling
occupancy_mainper building matches the intended flow and works withconfig.general.multiprocessing = False.
| def occupancy_main(locator: cea.inputlocator.InputLocator, config: cea.config.Configuration, building=None): | ||
| migrate_void_deck_data(locator) | ||
| # local variables | ||
| buildings: List[str] = config.occupancy_helper.buildings | ||
| occupancy_model: str = config.occupancy_helper.occupancy_model | ||
| buildings: List[str] = config.occupancy.buildings | ||
| occupancy_model: str = config.occupancy.occupancy_model | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename + config path switch look good; ensure [occupancy] section exists at runtime
The function rename and move to config.occupancy.* read cleanly. Please confirm the default/user config files define an occupancy section with buildings and occupancy_model, and that all callers/tests set these under occupancy (not occupancy_helper).
Run:
🏁 Script executed:
#!/bin/bash
# 1) Find any stale references that might break at runtime
rg -nP -C2 '\boccupancy_helper_main\b|\bconfig\.occupancy_helper\b|occupancy[_-]helper'
# 2) Verify [occupancy] section exists in config templates
rg -nP -n '^\[occupancy\]' -C2Length of output: 2451
Remove stale occupancy-helper references and introduce [occupancy] config section
- Rename config section: in
cea/default.config, change[occupancy-helper]to[occupancy]and ensurebuildingsandoccupancy_modelare defined. - Update CLI commands: in
cea/scripts.yml(around lines 175–179 and 185–189), replacename: occupancy-helperand related script entries with the new command (e.g.occupancy). - Adjust workflow code: in
cea/utilities/batch_process_workflow.py(line 122), update['cea', 'occupancy-helper', …]to['cea', 'occupancy', …]. - Fix tests: in
cea/tests/workflows/zug_heating.ymlandsg_cooling.yml(line 31), change- script: occupancy-helperto- script: occupancy.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cea/default.config (1)
322-334: Implement config migration for deprecated[occupancy-helper]
No code still readsoccupancy_helper(verified); add logic in your config‐formatting tool (e.g. write-config/format-helper) to rename users’[occupancy-helper]blocks to[occupancy].
🧹 Nitpick comments (1)
cea/scripts.yml (1)
178-178: Capitalize label for consistency.Use “Energy Demand Part 1: Building Occupancy” to match title casing used elsewhere.
- label: "Energy Demand Part 1: building occupancy" + label: "Energy Demand Part 1: Building Occupancy"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cea/default.config(1 hunks)cea/inputlocator.py(1 hunks)cea/scripts.yml(1 hunks)cea/tests/workflows/sg_cooling.yml(1 hunks)cea/tests/workflows/zug_heating.yml(1 hunks)cea/utilities/batch_process_workflow.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cea/inputlocator.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests (windows-latest)
- GitHub Check: tests (ubuntu-latest)
- GitHub Check: tests (macos-latest)
🔇 Additional comments (6)
cea/utilities/batch_process_workflow.py (1)
121-124: Verifiedcea occupancyCLI is in scripts.yml (line 177) and nooccupancy-helperreferences exist.cea/tests/workflows/sg_cooling.yml (1)
31-31: Workflow step renamed to occupancy — LGTM.Matches the CLI rename; ordering relative to radiation and demand remains correct.
cea/tests/workflows/zug_heating.yml (1)
31-31: Workflow step renamed to occupancy — LGTM.Consistent with the repository-wide rename.
cea/scripts.yml (3)
177-183: Entry rename and module target updated — good alignment.‘name: occupancy’ and ‘module: cea.demand.occupancy’ align with the new API.
189-194: InputLocator methods exist
Verified that get_building_architecture, get_zone_geometry, and get_weather_file are implemented in cea/inputlocator.py.
187-187: All consumers updated toconfig.occupancy; no lingeringoccupancy_helperoroccupancy-helperreferences detected.
|
Hi @martin-mosteiro |
Terrain input is not used by the occupancy script
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
cea/schemas.yml (7)
2406-2409: Mass assemblies: drop occupancy from used_by unless proven needed.No clear link between occupancy script and mass assembly DB.
Apply:
used_by: - demand - radiation - - occupancy
2454-2458: Floor assemblies: same concern—likely not used by occupancy.Apply:
used_by: - demand - radiation - - occupancy
2525-2529: Roof assemblies: remove occupancy from used_by if unused.Apply:
used_by: - demand - radiation - - occupancy
2557-2560: Shading assemblies: occupancy probably doesn’t consume this DB.Apply:
used_by: - demand - radiation - - occupancy
2588-2591: Tightness assemblies: same — recommend removing occupancy.Apply:
used_by: - demand - radiation - - occupancy
2658-2661: Wall assemblies: remove occupancy unless there’s a direct call.Apply:
used_by: - demand - radiation - - occupancy
2730-2733: Window assemblies: remove occupancy from used_by if unused.Apply:
used_by: - demand - radiation - - occupancy
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/schemas.yml(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests (macos-latest)
- GitHub Check: tests (windows-latest)
- GitHub Check: tests (ubuntu-latest)
🔇 Additional comments (7)
cea/schemas.yml (7)
19164-19165: Rename producer from occupancy_helper → occupancy looks correct.This aligns the schema’s
created_bywith the new module name.
1824-1825: OK to add occupancy as consumer of building comfort.Occupancy outputs include setpoints; dependency makes sense.
1918-1919: OK to add occupancy as consumer of building internal loads.Occupancy uses densities/people gains; this is accurate.
21039-21041: OK to add occupancy as consumer of weather file.Occupancy reads weather; dependency is correct.
21173-21178: OK to add occupancy as consumer of zone geometry.The module loads zone geometry; this is correct.
1769-1769: Keep occupancy in used_by — it's referenced by cea/demand/occupancy.py:54cea/demand/occupancy.py line 54:
architecture = pd.read_csv(locator.get_building_architecture()).set_index('name')— do not remove- occupancyfromused_by.Likely an incorrect or invalid review comment.
10569-10572: Likely removeoccupancyfrom envelope systems database — verify firstRepo search limited to cea/demand/occupancy.py returned no matches; usage not confirmed. If
occupancyis unused by envelope systems, remove it from cea/schemas.yml.Suggested change:
get_database_envelope_systems: ... used_by: - demand - radiation - - occupancyVerify (repo-wide):
rg -nP --glob '!**/venv/**' 'get_database_envelope_systems|locator\.get_database_envelope_systems' rg -nP --glob '!**/venv/**' '^\s*-\s*occupancy\b' cea/schemas.yml
On CEA, all the
_helpers are about retrieving data, except theoccupancy_helper, which includes occupancy models. I think this is a bit confusing, especially because it makes it harder to differentiateschedule_helperfromoccupancy_helper.Why not just call it
occupancy, since that is what the script is about? It's basically the same asthermal_loads(which isn'tthermal_loads_helper) orthermal_network(which isn'tthermal_network_helper).I think this makes things a bit clearer with no downsides!
Summary by CodeRabbit