Conversation
Added a Pelicun conversion method to `input_builder.py`. This commit is not yet functional due to incomplete simulated_damage.json outputs. Changes: - build_simulated_inputs() now searches for DMG_sample.csv as an indicator of Pelicun outputs - convert_pelicun() builds raw inputs using Pelicun outputs. - (hotfix) in build_inputs.py -> build_simulated_inputs(): - check for 'tenant_units' to populate now matches, and for 'story' vice versa - Under # Write in individual dictionaries... Input: - CMP_QNT.csv - DL_summary.csv - DMG_sample.csv - DV_repair_sample.csv - general_inputs.json - input.json Output: - building_model.json - damage_consequences.json - comp_ds_list.csv - comp_population.csv - simulated_damage.json
- Changed file structure to match src/ reorganization - Temporarily handled edge case where `repair_start_day` is all NaN by replacing it with 0.0 - Removed x-lim on recovery plot as it is tied to currently faulty "Full recovery day"
- driver.run_analysis(force_rebuild=True) will now rebuild simulated_inputs.json even if existing file is found.
…o pelicun converter
plotters/main_plot_functionality - Removed plotModel_PBEE_recovery.py (moved under plotters for future archiving) - Added plot_results() function to plotters.main_plot_functionality
- For input_builder.csv, damage for each side (affecting cladding) is randomly assigned a percentage summing up to 1.0 - Separated "tenant_units" and "story" from simulated_damage.json. "Story" now only includes the ["qnt_damaged_dir_n"] keys. Future implementation should consider allowing for side damage to be specified via general_inputs, e.g.
- Set seed occurs earlier to account for randomness in side damage (cladding) assignment
- Fixed overwrite in comp_population.csv to prevent resetting of component quantity if same component is listed multiple times in CMP_QNT.csv
- allow users to specify engineering cost as a fraction of repair cost in general_inputs.json - updated example `general_inputs.json` to reflect this
- removed json typo error - removed edge case NaN handling in other_plot_functions
- Allow users to specify `side_damage_ratio.csv`, an array to split cladding damage by direction - Elevators and stair count can now be overridden from `CMP_QNT.csv` using `general_input.json` - Default action for simultaneous damage state (elevators) is now to turn off the `is_sim_ds` flag without needing a custom `damage_state_attribute_mapping.csv`. Users are warned about simultaneous ds in multiple PGs and are recommended to change component. - Added robustness in cmp-loc-dir-ds metatag detection and reorder appropriately in DMG and DV samples. - doc: Added default and override instructions to README for the above.
zsarnoczay
left a comment
zsarnoczay
left a comment
There was a problem hiding this comment.
What I did
- Checked out the PR locally.
- Confirmed the example in the PR description runs end-to-end as written.
- Reviewed every file included in the PR, with an emphasis on Pelicun support (cli.py, driver.py, input_builder.py, README.md) and plotting utilities.
Summary
Overall, this PR looks solid and close to ready. I am not approving yet because there are a small number of blocking comments below that I believe should be addressed before merge. Most other items are non-blocking or future-looking and can be deferred.
Comment labels used below
- blocking: Must be addressed before merging (high-impact correctness/robustness, or very low-effort fixes that prevent likely user confusion).
- non-blocking: Nice-to-have improvements that could land in this PR, but I’m fine moving them to issues or a follow-up.
- future: Larger scope, design/documentation items, or work that warrants a dedicated issue. I included a link to the corresponding issue in each of these comments.
src/atc138/cli.py
Outdated
| parser.add_argument("input_dir", help="Path to the directory containing input files (e.g., simulated_inputs.json)") | ||
| parser.add_argument("output_dir", help="Path to the directory where outputs will be saved") | ||
| parser.add_argument("--seed", type=int, help="Random seed for reproducibility", default=None) | ||
| parser.add_argument("--force_rebuild", type=bool, help="Flag to force override of simulated_inputs.json and rebuild", default=False) |
There was a problem hiding this comment.
non-blocking:
Switch --force_rebuild to action='store_true' for ergonomic CLI behavior. With type=bool, users must pass a value (e.g., --force_rebuild True). The conventional approach is:
- parser.add_argument("--force_rebuild", action="store_true", help="…")
src/atc138/input_builder.py
Outdated
| # ds attributes | ||
| static_data_dir = os.path.join(os.path.dirname(__file__), 'data') | ||
|
|
||
| ds_attributes = pd.read_csv( |
There was a problem hiding this comment.
blocking:
Honor model-level overrides for damage_state_attribute_mapping.csv by using load_custom_static_tables(model_dir, static_data_dir, 'damage_state_attribute_mapping.csv') instead of loading directly from the packaged static directory. This aligns with how other static tables are loaded and enables per-model customizations. It’s a low-risk, high-value robustness fix and prevents confusing behavior when users provide an override that is silently ignored.
Proposed change (conceptual):
- static_data_dir = os.path.join(os.path.dirname(file), 'data')
- ds_attributes = load_custom_static_tables(model_dir, static_data_dir, 'damage_state_attribute_mapping.csv')
src/atc138/input_builder.py
Outdated
|
|
||
| # remove the units row | ||
| damage = damage[:-1] | ||
| dvs = dvs[:-1] |
There was a problem hiding this comment.
blocking:
Drop the "Units" row explicitly rather than by position when removing preamble rows from Pelicun CSVs. Position-based slicing can silently remove valid data if the file structure changes.
src/atc138/input_builder.py
Outdated
|
|
||
| with open(os.path.join(model_dir, 'input.json')) as file: | ||
| pelicun_inputs = json.load(file) | ||
| file.close() |
There was a problem hiding this comment.
non-blocking:
Remove the explicit file.close() when using with open(...). Context managers automatically close files even on exceptions. This is a small cleanup that reduces noise:
- with open(os.path.join(model_dir, 'input.json')) as file:
pelicun_inputs = json.load(file)no file.close() needed
Do the same for general_inputs.json below.
| df.columns = new_cols | ||
| return df | ||
|
|
||
| def story_mask(location, num_stories): |
There was a problem hiding this comment.
non-blocking:
Support 'top' as an alias for the roof story to improve Pelicun compatibility.
Also note that roof assigns components above the top story. For example, in a 3-story building, top assigns to story 3 while roof assigns to story 4. This is important to get the correct roof accelerations for equipment put on the roof slab as opposed to the slab of the top floor.
See the corresponding code in Pelicun for further details: https://github.com/NHERI-SimCenter/pelicun/blob/73111704f36022268cb8b87989c413b53af6f9f0/pelicun/model/pelicun_model.py#L328
There was a problem hiding this comment.
"top" and "roof" now both place the component at the highest location available. However, from the examples I checked, there are no component placed at locations above the top story (i.e. there is no explicit roof). To support this, n_stories buildings should be expanded to support n_stories+1 location, then roof components should be placed (and repaired/tagged) there accordingly.
Recommend this be classified as "future", tracked at issue #18
| - **systems.csv**: Attributes of each default ssytem considered in the method. | ||
| - **temp_repair_class.csv**: Attributes of each temprary repair class considered in the method. | ||
|
|
||
| ## Building from Pelicun outputs |
There was a problem hiding this comment.
non-blocking:
Polish the README Pelicun section to provide additional details and improve user experience:
- Note that if simulated_inputs.json exists, the builder will not re-run unless --force_rebuild is used.
- Mention that DV_bldg_repair_sample.csv is supported as a fallback name for decision variables.
There was a problem hiding this comment.
future:
Decide whether to keep this legacy (?) script. If retained, clearly mark it as legacy and align its behavior with the recommended entry point - currently it is not working properly; otherwise, remove it to reduce redundancy. No need to modify it in this PR.
Created #15 to track
There was a problem hiding this comment.
non-blocking:
- Add a short "Plotting outputs" note pointing to plotters/main_plot_functionality.plot_results(outputs_dir, p_gantt=50) as the recommended entry point.
- Add brief module-level docstrings clarifying roles of main_plot_functionality.py, other_plot_functions.py.
|
|
||
| from copy import deepcopy | ||
|
|
||
| with open(os.path.join(model_dir, 'input.json')) as file: |
There was a problem hiding this comment.
Consider removing input.json as a hard requirement by moving its few fields (number of stories, replacement cost, plan area) into general_inputs.json.
Created #16 to track
| # calculate repair cost ratio | ||
| # HP: check nomenclature "-" vs "_" | ||
|
|
||
| # HP: missing racked stair doors per story, racked entry doors |
- damage_state_attribute_mapping.csv loading now uses load_custom_static_tables method - drop the Units row now uses the first column (cmp-loc-dir indicator) to explicitly identify "unit" string (case insensitive) - removed file.close()
- Switched CLI --force_rebuild argument to store_true to add default=False behavior - Aliased "top" for "roof" in location assignment. This location is set to be the highest location in the building. e.g. for a 3-story building, both roof and top would place the component at [1, 2, (3)]. Recommend future expand to support explicit roof. - clean_frag_id refactored to split on '.' and rejoin in two parts - gated comp_population to only components verified in comp_ds_list - reuse clean_frag_id wherever period formatting happens - Removed nested import of re - Added --force_rebuild details to README - Added plotters detail to README - Added module-level docstring to plotters/main_plot_functionality and plotters/other_plot_functions.py
2 participants
Summary
Added a Pelicun conversion method to
input_builder.py. All major changes are implemented ininput_builder.convert_pelicun(), and core ATC-138 functions are unmodified.Key Changes
1. Support for Pelicun outputs as input files
input_builder.convert_pelicunwill be called if Pelicun files are found in a model directorybuild_simulated_inputs()now searches for DMG_sample.csv as an indicator of Pelicun outputsforce_rebuild_flagindriver.run_analysis()andcli.pyto allow overriding of existingsimulated_inputsexamples/RCSW_4story_pelicun2. Reorganized plotting module
plotters/main_plot_functionalityp_ganttas an argument to plot realizations other than median3. Hotfixes
build_inputs.py -> build_simulated_inputs(): check fortenant_unitsto populate now matches, and forstoryvice versa (Under# Write in individual dictionaries...)functionality.other_functionality_functions: Fixed typo insys.exit4. Documentation
README.mdto reflect the package changes.HP:comments5. Implementation notes
damage_state_attribute_mapping.csvto overrideis_sim_dsfor elevatorsVerification
RCSW_4story_pelicunexample:Plots generated