Conversation
…ath normalization
…lt library handling
…tine directory creation
…date data caching logic
…flect the card implementation in the SAMMY manual.
…dules and adding new ones
…inp05_broadening and inp07_density
…ut and material number
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive refactor of PLEIADES' configuration system, transitioning from a simple dataclass-based approach to a robust Pydantic-based hierarchical configuration management system. The changes enable structured validation, workspace-relative path expansion, and enhanced support for nuclear data and fit routine configuration.
Changes:
- Replaced
PleiadesConfigdataclass with Pydantic models (PleiadesConfig,WorkspaceConfig,NuclearConfig,SammyConfig,IsotopeConfig) providing automatic validation and type coercion - Implemented path expansion logic with workspace variable substitution (e.g.,
${workspace.root}) and environment variable support - Added new utility methods for building nuclear parameters, ensuring ENDF cache files, and creating timestamped routine directories
- Enhanced serialization using
model_dumpandmodel_validatewith YAML safe_dump for improved consistency - Updated unit tests to verify synchronization between legacy and new config fields
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| src/pleiades/utils/config.py | Complete refactor from dataclass to Pydantic models with new validation, path expansion, and utility methods |
| tests/unit/pleiades/utils/test_utils_config.py | Updated existing tests with assertions for nuclear config synchronization |
| docs/Notes/pleiades_config_workflow.md | New comprehensive YAML configuration specification and workflow documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…EIADES into fix/213-integration-of-fitconfig
KedoKudo
left a comment
There was a problem hiding this comment.
Manual testing passed on analysis cluster for ORNL notebooks as well as the configuration notebook.
In addition to the comments from my review, here are some issues identified by codex during its independent review:
PLEIADES/src/pleiades/sammy/io/inp_manager.py:335-340
- When InpManager is driven by fit_config (without explicit material_properties), this block builds the Card 5 source dict from broadening parameters but omits broadening.dist. The later fallback logic then uses 25.0 for flight path, so any non-default flight path configured in FitConfig is silently dropped and the generated INP contains incorrect Card 5 values.
PLEIADES/src/pleiades/sammy/io/inp_manager.py:809-812
- After parsing Card 5, the parser copies temperature and resolution terms into fit_config.physics_params.broadening_parameters but never assigns constants.flight_path_length to dist. This makes parse→reuse/generate flows non-roundtrippable: files with non-default flight path lose that value and can later be regenerated with the default path.
PLEIADES/src/pleiades/utils/config.py:489-491
- from_dict now always validates with require_fit_routines=True, but save() still permits writing configs with empty fit_routines (including default PleiadesConfig()). That means a config produced by this codepath can fail to load later, and existing legacy config files without fit_routines will also raise on load()/get_config() instead of being migrated or defaulted.
…set configuration
…che, fitting, results, data, and images
…() and making it strict
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 43 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
src/pleiades/sammy/io/card_formats/inp05_broadening.py:83
- The
Card05.from_linesmethod now accepts a line with only the temperature field (changed from requiring at least 2 fields to requiring at least 1 field). However, the method still has a default value for flight_path_length of 25.0 when the field is missing. This default is not validated and could silently produce incorrect results if users expect validation to catch missing required fields. Consider documenting this default clearly or making the field required if it's essential for SAMMY operation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request introduces a major refactor and enhancement of the global configuration management for PLEIADES, transitioning from a simple dataclass-based approach to a robust, hierarchical, and extensible Pydantic-based configuration system. The update includes a comprehensive new YAML configuration schema, improved path resolution and normalization, enhanced support for nuclear data and fit routines, and more thorough unit tests.
Key changes include:
Configuration System Overhaul
PleiadesConfigdataclass with a set of Pydantic models (PleiadesConfig,WorkspaceConfig,NuclearConfig,SammyConfig,IsotopeConfig) to provide structured, validated, and extensible configuration management. This enables hierarchical config sections, automatic type coercion, and custom validation logic.YAML Configuration and Documentation
Serialization and Deserialization Improvements
save()andto_dict()methods to use safe YAML dumping and preserve key order, and added afrom_dict()method for loading configurations from dictionaries. [1] [2]Nuclear Data and Fit Routine Handling
Unit Test Enhancements
These changes lay the groundwork for more maintainable, reproducible, and user-friendly configuration and workflow management in PLEIADES.
References: