Conversation
- Change `chemical_potential_range` default to None to avoid mutable default arrays. - Set `electron_tol` default to 0.05 for consistency. - Exit immediately with ValueError if chemical potential cannot be found in the specified range.
There was a problem hiding this comment.
Pull request overview
This PR refactors the documentation and improves the code quality of the thermal electronic modules. The changes focus on standardizing docstring formats, improving parameter descriptions, enhancing error handling, and adding inline comments for better code clarity.
Changes:
- Standardized docstring format across both files to use single-line Returns and Args sections without type annotations (relying on function signature type hints instead)
- Enhanced class-level documentation with detailed Attributes sections and improved descriptions
- Improved error handling by converting print statements to proper ValueError exceptions with exception chaining
- Added inline comments to clarify calculation steps
- Adjusted default parameters for chemical potential calculations (electron_tol: 0.5 → 0.05, chemical_potential_range now optional)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dfttk/thermal_electronic/thermal_electronic_data.py | Removed module docstring, enhanced class docstring with Args section, standardized method documentation format, reordered parameters in get_thermal_electronic_data, removed unused electron_dos_data attribute |
| dfttk/thermal_electronic/thermal_electronic.py | Removed TODO comment, standardized attribute and method documentation format, improved error handling in calculate_chemical_potential, added inline comments explaining calculation steps, changed default parameter values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| natural order. | ||
| Returns: List of folder names with the given prefix, sorted in natural order. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Multiple lines in this file contain trailing whitespace (e.g., lines 128, 147, 270, 636, 794, 847). While this is a minor style issue, consider running a linter or formatter to remove trailing whitespace for consistency with standard Python style guidelines.
| volumes_fit: np.ndarray, | ||
| temperatures: np.ndarray, | ||
| order: int = 1, | ||
| selected_volumes: np.ndarray = None, | ||
| folder_prefix: str = "elec", | ||
| vasprun_name: str = "vasprun.xml.elec_dos", | ||
| selected_volumes: np.ndarray = None, |
There was a problem hiding this comment.
The parameter order has been changed by moving 'selected_volumes' from before 'folder_prefix' to after 'vasprun_name'. This could break external code that calls this method with positional arguments. The internal caller in configuration.py uses keyword arguments and won't be affected, but consider whether this is a public API that external users might depend on. If so, this breaking change should be documented or avoided.
1 participant
No description provided.