Develop #76
Develop #76
Conversation
…ons, add `per_charge`, `per_site` and `skip_formatting` options to `get_quenched_fermi_level_and_concentrations` as well, plus some general efficiency/robustness updates, and add tests
… block in new `pymatgen`)
… better), make oxi-guessing functions public, use `SimpleQueue` to imporove timeout efficiency...
WalkthroughThe recent updates primarily focus on improving oxidation state handling, enhancing defect concentration calculations, and ensuring compatibility with the latest releases of Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant DopedAnalysis
participant DopedCore
participant Pymatgen
User->>DopedAnalysis: Initialize defect analysis
DopedAnalysis->>DopedCore: Request oxidation state guess
DopedCore->>Pymatgen: Use BVAnalyzer for oxidation state
Pymatgen-->>DopedCore: Return oxidation state
DopedCore-->>DopedAnalysis: Set oxidation state
DopedAnalysis-->>User: Provide defect analysis results
Poem
Warning Review ran into problemsProblems (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (6)
docs/Dev_ToDo.md (4)
Line range hint
6-6: Consider adding a comma after "Also".
Line range hint
15-15: Consider replacing "outside of" with "outside" to avoid redundancy.
Line range hint
16-16: Incomplete sentence detected. Consider revising to include a subject.- Would be easy to implement if auto degeneracy... + It would be easy to implement if auto degeneracy...
Line range hint
19-19: The phrase "want plotting tools like this" seems to be missing a verb. Consider revising for clarity.- want plotting tools like this + we want plotting tools like thistests/test_analysis.py (2)
Line range hint
2694-2735: Consider refactoring the repeated code for file operations and XML modifications into helper functions.To improve code maintainability and reduce redundancy, consider creating helper functions for common operations like moving files, modifying XML content, and setting up test environments. This will make the test cases cleaner and easier to understand.
Line range hint
2763-2795: Ensure consistent error handling for file operations.The code performs file operations (like moving and copying files) without explicit error handling. It's recommended to add error handling to prevent the test from failing unexpectedly in cases where file operations do not succeed.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- CHANGELOG.rst (1 hunks)
- docs/Dev_ToDo.md (1 hunks)
- docs/Troubleshooting.rst (1 hunks)
- doped/analysis.py (3 hunks)
- doped/core.py (9 hunks)
- doped/generation.py (6 hunks)
- doped/thermodynamics.py (16 hunks)
- doped/vasp.py (33 hunks)
- examples/GGA_workflow_tutorial.ipynb (5 hunks)
- pyproject.toml (1 hunks)
- tests/data/VO2_POSCAR (1 hunks)
- tests/test_analysis.py (8 hunks)
- tests/test_corrections.py (2 hunks)
- tests/test_generation.py (6 hunks)
- tests/test_thermodynamics.py (4 hunks)
- tests/test_vasp.py (2 hunks)
Files skipped from review due to trivial changes (4)
- CHANGELOG.rst
- docs/Troubleshooting.rst
- pyproject.toml
- tests/data/VO2_POSCAR
Additional context used
LanguageTool
docs/Dev_ToDo.md
[uncategorized] ~3-~3: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: .... Propervasp_stdsetup (withNKREDfolders like for defect calcs) andvasp_nclg...
[uncategorized] ~6-~6: A comma may be missing after the conjunctive/linking adverb ‘Also’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...of code out ofinterfacesubpackage?) Also can see https://github.com/materialspro...
[style] ~7-~7: Two successive sentences begin with the same adverb. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: .../phases.py for 2D chempot plotting. - Also seeCs2SnTiI6notebooks for template ...
[style] ~15-~15: This phrase is redundant. Consider using “outside”. (OUTSIDE_OF)
Context: ...e plot - Don't show transition levels outside of the bandgap (or within a certain range ...
[style] ~16-~16: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...iscussion and CdTe pyscfermi notebooks. Would be easy to implement if auto degeneracy...
[grammar] ~19-~19: If ‘want’ is used as a verb, it usually requires the infinitive. (AFFORD_VBG)
Context: ...a of theAiiDA-defectspreprint, want plotting tools like this - Can we add an option ...
[typographical] ~20-~20: Except for inverted sentences, ‘Can we’ requires a question mark at the end of the sentence. (MD_PRP_QUESTION_MARK)
Context: ...defect-structures) – seems quite useful tbf ## Housekeeping - Tutorials general st...
[style] ~33-~33: In American English, abbreviations like “etc.” require a period. (ETC_PERIOD)
Context: ...state behaviour being a bit more common etc). If the user wants to add bandfilling ...
[grammar] ~33-~33: Modal verbs like ‘can’ or ‘will’ require the following verb to be in its base form. (MD_BASEFORM)
Context: ...bandfilling corrections, they can still doing this by calculating it themselves and a...
[style] ~40-~40: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ... of a VASP dielectric calculation if an oddly-defined primitive cell is used) -vasp_ncl...
[formatting] ~42-~42: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause. (COMMA_BEFORE_BECAUSE)
Context: ... Often can't useNKREDwithvasp_std, because we don't know beforehand the kpts in th...
[style] ~44-~44: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE)
Context: ...kpoints, due to memory limitations. - Readily-usable in conjunction withatomate,AiiDA(...
[style] ~44-~44: ‘in conjunction with’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
Context: ... memory limitations. - Readily-usable in conjunction withatomate,AiiDA(-defects),vise, `...
[typographical] ~46-~46: Do not use a colon (:) before a series that is introduced by a preposition (‘with’). Remove the colon or add a noun or a noun phrase after the preposition. (RP_COLON)
Context: ...onal dependencies. - Workflow diagram with: https://twitter.com/Andrew_S_Rosen/stat...
[style] ~47-~47: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase. (EN_WEAK_ADJECTIVE)
Context: ...an sometimes be worth doing if you have a very large supercell for speed up, but it's impo...
[style] ~47-~47: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase. (EN_WEAK_ADJECTIVE)
Context: ...only do if you're a power user and have a very large supercell. - Show usage of `get_conv_...
[style] ~51-~51: In American English, abbreviations like “etc.” require a period. (ETC_PERIOD)
Context: ...put structure,generate_supercelletc), but conventional cell should always b...
[uncategorized] ~54-~54: Possible missing article found. (AI_HYDRA_LEO_MISSING_AN)
Context: ...CarrierCapture.jlandnonrad. Show example of usingsumoto get the DOS plot...
[style] ~57-~57: This phrase is redundant. Consider writing “cancel”. (CANCEL_OUT)
Context: ...ts that the SOC effects on total energy cancel out for chemical potential calculations...
[uncategorized] ~66-~66: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...he parsed defect data in a lean format. Also using theget_formation_energiesetc....
[style] ~69-~69: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident. (SOMEWHAT)
Context: ...ral, the correction terms should follow somewhat consistent trends (for a given charge s...
[grammar] ~69-~69: It appears that only one verb is needed. (THIS_IS_HAVE)
Context: ... something odd is happening there. This is can be fairly easily scanned with `get_form...
[uncategorized] ~70-~70: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...The Wyckoff analysis code is very useful and no other package can do this afaik. See...
[typographical] ~75-~75: Consider adding a comma here. (PLEASE_COMMA)
Context: ...s and can fail in weird cases. As always please consider if these charge states are ...
[style] ~76-~76: In American English, abbreviations like “etc.” require a period. (ETC_PERIOD)
Context: ...mmetry, amphoteric, mixed-valence cases etc!) - Note cases where we expect defa...
[style] ~77-~77: Consider changing the order of words to improve your wording. (TO_NOT_VB)
Context: ...s where we expect default charge states to not be appropriate (e.g. mixed ionic-covale...
[style] ~82-~82: In American English, abbreviations like “etc.” require a period. (ETC_PERIOD)
Context: ...tum/magnetic defects, d-orbital defects etc), in which case you can manually adjust...
[style] ~84-~84: In American English, abbreviations like “etc.” require a period. (ETC_PERIOD)
Context: ...lay around withprobability_thresholdetc). - Should flick through other defect c...
[style] ~93-~93: In American English, abbreviations like “etc.” require a period. (ETC_PERIOD)
Context: ...lso group defects for the concentration etc functions, currently doesn't (e.g. `CdT...
Additional comments not posted (49)
tests/test_corrections.py (2)
17-17: Updated imports to reflect changes in the main codebase.
32-32: Class inheritance changed tounittest.TestCaseto standardize testing framework usage.docs/Dev_ToDo.md (1)
Line range hint
3-3: Consider adding a comma after "NKRED folders like for defect calcs".#!/bin/bash # Description: Verify if the comma is necessary. # Test: Check grammar rules for lists in technical documentation. echo "Grammar check completed."tests/test_vasp.py (21)
Line range hint
27-32: Method_potcars_availablecorrectly checks for the availability of POTCARs.
Line range hint
34-42: Method_check_potcar_dir_not_setup_warning_errorcorrectly constructs and checks warning messages.
Line range hint
44-51: Method_check_no_potcar_available_warning_errorcorrectly checks for specific substrings in a warning message.
Line range hint
53-57: Method_check_nelect_nupdown_errorcorrectly checks for a specific error message.
Line range hint
59-67: Method_check_nupdown_neutral_cell_warningcorrectly checks for specific substrings in a warning message.
Line range hint
69-92: MethodsetUpcorrectly initializes the test environment forDefectDictSetTest.
Line range hint
94-97: MethodtearDowncorrectly cleans up the test environment after tests are run.
Line range hint
99-141: Method_general_defect_dict_set_checkcorrectly implements comprehensive checks on aDefectDictSetinstance.
Line range hint
143-150: Method_check_potcar_nupdown_dds_warningscorrectly checks for specific warnings in aDefectDictSet.
Line range hint
152-192: Method_check_dds_incarcorrectly checks the INCAR settings of aDefectDictSet.
Line range hint
194-206: Method_check_dds_incar_and_writing_warningscorrectly checks for warnings during INCAR writing in aDefectDictSet.
Line range hint
208-234: Method_check_ddscorrectly implements comprehensive checks on aDefectDictSet.
Line range hint
236-250: Method_generate_and_check_ddscorrectly generates and checks aDefectDictSet.
Line range hint
252-261: Methodkpts_nelect_nupdown_checkcorrectly checks the k-points, NELECT, and NUPDOWN settings of aDefectDictSet.
Line range hint
263-277: Methodtest_neutral_defect_dict_setcorrectly tests the behavior of aDefectDictSetwith a neutral defect.
Line range hint
279-293: Methodtest_charged_defect_incarcorrectly tests the behavior of aDefectDictSetwith a charged defect.
Line range hint
295-314: Methodtest_user_settings_defect_incarcorrectly tests the behavior of aDefectDictSetwith custom user INCAR settings.
Line range hint
316-332: Methodtest_bad_incar_settingcorrectly tests the behavior of aDefectDictSetwhen incorrect INCAR settings are provided.
Line range hint
334-365: Methodtest_initialisation_for_all_structscorrectly tests the initialization of aDefectDictSetfor various structures.
Line range hint
367-385: Methodtest_file_writing_with_without_POTCARscorrectly tests the file writing behavior of aDefectDictSetwhen POTCARs are and are not available.
Line range hint
387-457: Method_write_and_check_dds_filescorrectly writes files for aDefectDictSetand performs checks on the written files.doped/core.py (3)
10-10: Update toSimpleQueuefromQueueshould be verified for compatibility.#!/bin/bash # Description: Verify that SimpleQueue is used appropriately throughout the codebase. # Test: Search for the usage of SimpleQueue. Expect: Proper usage without issues. rg --type python 'SimpleQueue'
1273-1294: Ensure proper error handling when guessing oxidation states.#!/bin/bash # Description: Verify that error handling for oxidation state guessing is robust. # Test: Search for the usage of `guess_and_set_struct_oxi_states`. Expect: Proper error handling. rg --type python 'guess_and_set_struct_oxi_states'
1369-1416: Verify the implementation of timeouts in oxidation state guessing.#!/bin/bash # Description: Verify the implementation of timeouts in oxidation state guessing. # Test: Search for the usage of `guess_and_set_oxi_states_with_timeout`. Expect: Proper implementation of timeouts. rg --type python 'guess_and_set_oxi_states_with_timeout'doped/generation.py (3)
43-43: Updated import to use the new function nameguess_and_set_oxi_states_with_timeout.This change aligns with the refactoring noted in the PR summary, ensuring consistency across the codebase.
1344-1345: Implementation ofguess_and_set_oxi_states_with_timeoutto handle oxidation states.This update is part of the refactoring efforts to improve the handling of oxidation states, making the process potentially more robust and efficient by incorporating a timeout mechanism.
1333-1333: Change in default behavior for_bulk_oxi_statesfromTruetoFalse.- False # to check if pymatgen can guess the bulk oxidation states + True # to check if pymatgen can guess the bulk oxidation statesThis change might affect the default behavior of oxidation state handling in various parts of the code. It's crucial to ensure that this change is intentional and documented, especially if it affects public APIs or expected outputs.
Likely invalid or redundant comment.
doped/analysis.py (2)
29-29: Updated import to use the public version ofguess_and_set_oxi_states_with_timeout.
709-716: Correctly implemented oxidation state setup with timeout handling.Consider adding error handling or logging to capture and respond to potential failures in the oxidation state guessing process.
#!/bin/bash # Description: Verify error handling in oxidation state setup. # Test: Search for error handling related to oxidation state setup. Expect: Some form of error handling or logging. rg --type python 'error handling|logging' 'doped/analysis.py'doped/vasp.py (10)
23-23: The import ofVaspInputSetfrompymatgen.io.vasp.setshas been updated to reflect the changes in the underlying library or to simplify the import structure. This change should be cross-verified with thepymatgenlibrary's latest documentation to ensure compatibility.
139-139: The parameteruser_potcar_functionalin the__init__method ofDopedDictSetis now explicitly typed asstr. This is a good practice as it enhances code readability and type safety.
320-320: InDefectDictSet, theuser_potcar_functionalparameter is consistently typed asstracross different classes, which maintains consistency and type safety in the codebase.
72-72: The function_test_potcar_functional_choicehas been updated to handleRuntimeError, which is a change in thepymatgenlibrary version 2024.5.1. Ensure that this version or higher ofpymatgenis specified in the project dependencies to avoid runtime issues.#!/bin/bash # Ensure the correct pymatgen version is installed pip show pymatgen | grep "Version: 2024.5.1"
126-126: The classDopedDictSethas been updated to inherit fromVaspInputSetinstead ofDictSet. This change likely aligns with updates in thepymatgenlibrary. Confirm that all methods and properties ofDictSetthat are used inDopedDictSetare compatible or appropriately overridden.#!/bin/bash # Check for any missing methods or properties in VaspInputSet that are used in DopedDictSet grep -R "DictSet" ./src | grep -v "DopedDictSet"
208-208: The method__init__inDopedDictSetnow includes a more structured approach to handlingPOTCARsettings, which improves the flexibility and maintainability of the code. This change should be tested to ensure that it does not affect the expected behavior of the system.#!/bin/bash # Run unit tests related to POTCAR settings pytest -k "test_potcar_settings"
224-224: The methodincarinDopedDictSetnow includes logic to handleKPARsettings based on the number of k-points. This is a significant enhancement for performance optimization in VASP calculations. Ensure that this logic is covered by unit tests to prevent regressions in future changes.#!/bin/bash # Verify the KPAR settings logic with unit tests pytest -k "test_kpar_settings"
252-252: The methodpotcarinDopedDictSetnow dynamically adjusts thePOTCARfunctional based on availability, which is a robust enhancement. It's crucial to ensure that this functionality is compatible with all expected use cases in the project.#!/bin/bash # Check compatibility of dynamic POTCAR functional adjustment pytest -k "test_dynamic_potcar_functional"
310-310: The classDefectDictSetextendsDopedDictSetto handle VASP defect calculations. This structural change should be reviewed to ensure that it correctly implements all necessary modifications for defect-specific calculations.#!/bin/bash # Review the implementation of DefectDictSet for correctness grep -R "DefectDictSet" ./src
494-494: The methodwrite_inputinDefectDictSethas been refactored to better handlePOTCARsetup checks. This is a critical update that enhances the robustness of the input file generation process. It's important to ensure that these changes are well-documented and tested.#!/bin/bash # Ensure the new POTCAR setup checks are functioning as expected pytest -k "test_write_input_potcar_checks"doped/thermodynamics.py (1)
1297-1298: Verify the aggregation logic in_group_defect_charge_state_concentrations.The method
_group_defect_charge_state_concentrationsis used here to aggregate concentrations. Ensure that this method correctly handles different cases, especially edge cases where defects might not be grouped as expected.#!/bin/bash # Description: Verify the aggregation logic by checking the method's handling of edge cases. # Test: Run unit tests related to this method. Expect: All tests pass. pytest tests/test_thermodynamics.py -k test_group_defect_charge_state_concentrationstests/test_generation.py (6)
Line range hint
1-6: The updated import statements are appropriate for the file's functionality.
297-297: Correct usage ofStructure.from_ase_atomsto convert ASE atoms to a pymatgen structure.
608-629: The defect generation information is detailed and correctly formatted.
1010-1022: Robust handling of warnings and assertions related to structure dimensions during defect generation.
1773-1779: Assertions correctly check thebulk_site_concentrationagainst expected values, ensuring the accuracy of defect calculations.
3212-3216: The assertion ensures that the output contains the expected defect information, validating the test results.
| # test per_site and bulk_site_concentration attributes: | ||
| new_conc = random_defect_entry.equilibrium_concentration( | ||
| chempots=self.CdTe_chempots, | ||
| limit="Cd-rich", | ||
| fermi_level=fermi_level, | ||
| temperature=temperature, | ||
| per_site=True, | ||
| ) | ||
| assert np.isclose(orig_conc, new_conc * random_defect_entry.bulk_site_concentration) |
There was a problem hiding this comment.
Consider adding a unit test for the per_site parameter in equilibrium_concentration.
It seems that the per_site parameter's effect on the output is not directly tested. Would you like me to help by writing a unit test for this scenario?
| @@ -2669,6 +2825,7 @@ def test_bulk_defect_compatibility_checks(self): | |||
| charge_state=+1, # manually specify charge state here, as our edited POTCAR doesn't exist | |||
| parse_projected_eigen=False, | |||
| ) | |||
| print([str(warning.message) for warning in w]) # for debugging | |||
There was a problem hiding this comment.
Add cleanup procedures in tests to ensure no side effects for subsequent tests.
The test modifies global state (e.g., moving and editing files) but does not include cleanup code to restore the original state after the test completes. This can affect other tests that rely on the same files. Consider using setUp and tearDown methods or Python's with statement to manage resources safely.
| return self.bulk_site_concentration * degeneracy_factor * exp_factor | ||
|
|
There was a problem hiding this comment.
Consider adding a default value for bulk_site_concentration to handle potential None values.
- return self.bulk_site_concentration * degeneracy_factor * exp_factor
+ return (self.bulk_site_concentration or 0) * degeneracy_factor * exp_factorCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return self.bulk_site_concentration * degeneracy_factor * exp_factor | |
| return (self.bulk_site_concentration or 0) * degeneracy_factor * exp_factor |
| Whether to return the concentrations as percent concentrations per site, | ||
| rather than the default of per cm^3. (default: False) | ||
| skip_formatting (bool): | ||
| Whether to skip formatting the defect charge states and concentrations as | ||
| strings (and keep as ints and floats instead). (default: False) | ||
| strings (and keep as ``int``\s and ``float``\s instead). (default: False) |
There was a problem hiding this comment.
Consider adding a more detailed docstring for the get_equilibrium_concentrations method.
The method get_equilibrium_concentrations lacks a detailed explanation of its parameters and return type in the docstring. This could lead to confusion for future developers or users of the code. Consider adding a more detailed description, including the types and purposes of each parameter and what the method returns.
| @@ -1252,7 +1252,7 @@ | |||
| fermi_level=fermi_level, | |||
| vbm=defect_entry.calculation_metadata.get("vbm", self.vbm), | |||
| ) | |||
| concentration = defect_entry.equilibrium_concentration( | |||
| raw_concentration = defect_entry.equilibrium_concentration( | |||
There was a problem hiding this comment.
Ensure consistent naming for concentration-related variables.
The variable raw_concentration is used here, but similar variables in other parts of the code use different naming conventions (e.g., Raw Concentration). It's important to maintain consistent naming conventions to improve code readability and maintainability.
| "Raw Concentration": raw_concentration, | ||
| ( | ||
| "Concentration (per site)" if per_site else "Concentration (cm^-3)" | ||
| ): _format_concentration( |
There was a problem hiding this comment.
Optimize the concentration formatting logic.
The current implementation for formatting concentrations is somewhat repetitive and could be simplified. Consider creating a helper function that handles the formatting based on the per_site and skip_formatting flags. This would make the code cleaner and reduce redundancy.
- "Concentration (per site)" if per_site else "Concentration (cm^-3)"
+ format_concentration_label(per_site)And define format_concentration_label as:
def format_concentration_label(per_site):
return "Concentration (per site)" if per_site else "Concentration (cm^-3)"| def guess_and_set_struct_oxi_states(structure, try_without_max_sites=False): | ||
| """ | ||
| Tries to guess (and set) the oxidation states of the input structure, first | ||
| using the ``pymatgen`` ``BVAnalyzer`` class, and if that fails, using the | ||
| ICSD oxidation state probabilities to guess. | ||
|
|
||
| Args: | ||
| structure (Structure): The structure for which to guess the oxidation states. | ||
| try_without_max_sites (bool): | ||
| Whether to try to guess the oxidation states | ||
| without using the ``max_sites=-1`` argument (``True``)(which attempts | ||
| to use the reduced composition for guessing oxi states) or not (``False``), | ||
| when using the ICSD oxidation state probability guessing. | ||
|
|
||
| Returns: | ||
| Structure: The structure with oxidation states guessed and set, or ``False`` | ||
| if oxidation states could not be guessed. | ||
| """ | ||
| if structure_with_oxi := _guess_and_set_struct_oxi_states(structure): | ||
| return structure_with_oxi | ||
|
|
||
| return _guess_and_set_struct_oxi_states_icsd_prob(structure, try_without_max_sites) | ||
|
|
There was a problem hiding this comment.
Refactor to combine similar functions for setting oxidation states.
- def guess_and_set_struct_oxi_states(structure, try_without_max_sites=False):
- if structure_with_oxi := _guess_and_set_struct_oxi_states(structure):
- return structure_with_oxi
- return _guess_and_set_struct_oxi_states_icsd_prob(structure, try_without_max_sites)
+ def guess_and_set_struct_oxi_states(structure, try_without_max_sites=False):
+ return _guess_and_set_struct_oxi_states(structure) or _guess_and_set_struct_oxi_states_icsd_prob(structure, try_without_max_sites)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def guess_and_set_struct_oxi_states(structure, try_without_max_sites=False): | |
| """ | |
| Tries to guess (and set) the oxidation states of the input structure, first | |
| using the ``pymatgen`` ``BVAnalyzer`` class, and if that fails, using the | |
| ICSD oxidation state probabilities to guess. | |
| Args: | |
| structure (Structure): The structure for which to guess the oxidation states. | |
| try_without_max_sites (bool): | |
| Whether to try to guess the oxidation states | |
| without using the ``max_sites=-1`` argument (``True``)(which attempts | |
| to use the reduced composition for guessing oxi states) or not (``False``), | |
| when using the ICSD oxidation state probability guessing. | |
| Returns: | |
| Structure: The structure with oxidation states guessed and set, or ``False`` | |
| if oxidation states could not be guessed. | |
| """ | |
| if structure_with_oxi := _guess_and_set_struct_oxi_states(structure): | |
| return structure_with_oxi | |
| return _guess_and_set_struct_oxi_states_icsd_prob(structure, try_without_max_sites) | |
| def guess_and_set_struct_oxi_states(structure, try_without_max_sites=False): | |
| """ | |
| Tries to guess (and set) the oxidation states of the input structure, first | |
| using the ``pymatgen`` ``BVAnalyzer`` class, and if that fails, using the | |
| ICSD oxidation state probabilities to guess. | |
| Args: | |
| structure (Structure): The structure for which to guess the oxidation states. | |
| try_without_max_sites (bool): | |
| Whether to try to guess the oxidation states | |
| without using the ``max_sites=-1`` argument (``True``)(which attempts | |
| to use the reduced composition for guessing oxi states) or not (``False``), | |
| when using the ICSD oxidation state probability guessing. | |
| Returns: | |
| Structure: The structure with oxidation states guessed and set, or ``False`` | |
| if oxidation states could not be guessed. | |
| """ | |
| return _guess_and_set_struct_oxi_states(structure) or _guess_and_set_struct_oxi_states_icsd_prob(structure, try_without_max_sites) |
| lambda x: f"{x:.2%}" | ||
| ) | ||
| conc_df = conc_df.drop(columns=["Raw Charge", "Raw Concentration"]) | ||
| return conc_df.reset_index(drop=True) | ||
|
|
||
| # group by defect and sum concentrations: | ||
| summed_df = conc_df.groupby("Defect").sum(numeric_only=True) | ||
| summed_df[next(k for k in conc_df.columns if k.startswith("Concentration"))] = ( | ||
| summed_df["Raw Concentration"] | ||
| if skip_formatting | ||
| else summed_df["Raw Concentration"].apply(lambda x: f"{x:.3e}") | ||
| ) | ||
| return summed_df.drop( | ||
| columns=[ | ||
| i | ||
| for i in ["Charge", "Formation Energy (eV)", "Raw Charge", "Raw Concentration"] | ||
| if i in summed_df.columns | ||
| ] | ||
| return _group_defect_charge_state_concentrations( |
There was a problem hiding this comment.
Refactor the lambda function for clarity.
The lambda function used in the transformation of Charge State Population could be refactored into a named function for better readability and potential reuse.
- lambda x: f"{x:.2%}"
+ def format_percentage(value):
+ return f"{value:.2%}"Then replace the lambda with format_percentage.
Committable suggestion was skipped due to low confidence.
| @@ -106,8 +105,7 @@ def setUp(self): | |||
| atoms = bulk("Cu") | |||
| atoms = make_supercell(atoms, [[2, 0, 0], [0, 2, 0], [0, 0, 2]]) | |||
| atoms.set_chemical_symbols(["Cu", "Ag"] * 4) | |||
| aaa = AseAtomsAdaptor() | |||
| self.agcu = aaa.get_structure(atoms) | |||
| self.agcu = Structure.from_ase_atoms(atoms) | |||
There was a problem hiding this comment.
Replace Structure.from_ase_atoms(atoms) with Structure.from_atoms(atoms) for consistency.
- self.agcu = Structure.from_ase_atoms(atoms)
+ self.agcu = Structure.from_atoms(atoms)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| self.agcu = Structure.from_ase_atoms(atoms) | |
| self.agcu = Structure.from_atoms(atoms) |
No description provided.