-
Notifications
You must be signed in to change notification settings - Fork 4
Calculate thermal conductivity with phono3py #450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughA new file has been added to the workflows package for thermal conductivity analysis in materials. This file introduces two helper functions that generate phonon displacement structures and analyze structure forces to compute thermal conductivity. Additionally, a workflow class wraps these functionalities to provide a structured interface for initializing parameters, generating simulation tasks, and processing results. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Caller
participant CW as ConductivityWorkflow
participant Gen as generate_structures_helper
participant Ana as analyse_structures_helper
User->>CW: Instantiate with structure and parameters
User->>CW: Call generate_structures()
CW->>Gen: Invoke generate_structures_helper()
Gen-->>CW: Return task dictionaries and counts
User->>CW: Call analyse_structures(forces_lst)
CW->>Ana: Invoke analyse_structures_helper()
Ana-->>CW: Return thermal conductivity results
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
atomistics/workflows/conductivity.py (5)
1-2
: Add docstring for new module.Consider adding a high-level docstring at the top of the file, explaining the purpose of this new module. This fosters clarity and ease of discovery.
12-27
: Ensure docstrings and parameter descriptions are provided.
generate_structures_helper
is crucial for preparing supercell structures. It would benefit from docstrings describing each parameter (e.g.,phonon_supercell_matrix
,SNF_coordinates
, etc.), highlighting valid value ranges, and referencing external definitions (like Phono3py's documentation).
56-65
: Consider capturing shape mismatches in force calculations.When
phono.generate_displacements()
expands your structure, the shapes of the returned supercells are expected to match. A shape mismatch in subsequent code may obscure or break the force calculations. Add checks or exception handling to detect misalignments early.
89-106
: Enhance workflow with docstring and usage examples.
ConductivityWorkflow
is a public-facing workflow, yet it lacks docstrings. Include a class-level docstring explaining how to instantiate the workflow and the meaning of each parameter. Consider usage examples for clarity.
125-155
: Return descriptive results or logs inanalyse_structures
.While the method returns a dictionary with temperature and kappa, additional data (e.g., logs, warnings, or summary info) could help end-users troubleshoot. Provide more context in the returned dictionary or logs to clarify how the final conductivity was derived.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
atomistics/workflows/conductivity.py
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: unittest_old
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.10)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_matrix (macos-latest, 3.12)
- GitHub Check: unittest_siesta
- GitHub Check: minimal
- GitHub Check: notebooks
- GitHub Check: unittest_abinit
- GitHub Check: unittest_gpaw
- GitHub Check: pip_check
- GitHub Check: unittest_orb
- GitHub Check: unittest_mace
- GitHub Check: unittest_qe
- GitHub Check: unittest_grace
- GitHub Check: unittest_matgl
- GitHub Check: coverage
🔇 Additional comments (2)
atomistics/workflows/conductivity.py (2)
71-86
: Warn about validated force array shapes.
analyse_structures_helper
blindly assignsforces_lst
slices tophono.forces
andphono.phonon_forces
. Ensure that each array inforces_lst
has the correct shape for second and third-order force constants. Consider logging or validating dimensions if there's a possibility of user mistakes.
28-32
:❓ Verification inconclusive
Validate the default
primitive_matrix
computation.Normalizing based on the mean of the diagonal elements (
np.diag(structure.cell.array).mean()
) can cause unexpected distortions. Verify that this normalization aligns with your intended crystal orientation and scaling needs.
🏁 Script executed:
#!/bin/bash # Checking for references to primitive_matrix usage outside this file: rg --context 3 'primitive_matrix'Length of output: 23772
Revisit Default Primitive Matrix Normalization
The current approach in
atomistics/workflows/conductivity.py
computes the defaultprimitive_matrix
asif primitive_matrix is None: primitive_matrix = ( structuretoolkit.analyse.get_primitive_cell(structure).cell.array / np.diag(structure.cell.array).mean() )while this may work for nearly isotropic cells, normalizing by the mean of the diagonal elements can lead to unexpected distortions for anisotropic or non-orthogonal cells. Note that in some of our notebooks (e.g., in
notebooks/lammps_workflows.ipynb
), normalization is performed by dividing by the cube-root of the cell volume. Please validate that the chosen mean-based normalization meets your intended crystal orientation and scaling requirements, or consider harmonizing it with the volume-based approach if needed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
=======================================
Coverage 85.17% 85.17%
=======================================
Files 42 42
Lines 2563 2563
=======================================
Hits 2183 2183
Misses 380 380 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit