Closed editorialbot closed 2 months ago
editorialbot
commented
4 months ago Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfReference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1039/D3TA00532A is OK
- 10.1021/acsaem.2c03466 is OK
- 10.26434/chemrxiv-2024-hm6vh is OK
- 10.1038/s41467-022-32669-3 is OK
- 10.1021/acs.inorgchem.3c01510 is OK
- 10.1021/acs.jpclett.2c02436 is OK
- 10.1039/D2FD00043A is OK
- 10.1021/acsenergylett.1c00380 is OK
- 10.21105/joss.02102 is OK
- 10.1039/D1SC03775G is OK
- 10.1103/PRXEnergy.2.043002 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.26434/chemrxiv-2023-8l7pb is OK
- 10.1021/acs.jpcc.3c05204 is OK
- 10.1038/s41524-023-00973-1 is OK
- 10.1039/D3CS00432E is OK
- 10.1016/j.matt.2021.06.003 is OK
- 10.21105/joss.04817 is OK
- 10.1088/2516-1075/ace014 is OK
- 10.1016/j.cpc.2018.01.011 is OK
- 10.1016/j.commatsci.2022.111434 is OK
- 10.1039/D3TA02429F is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.21105/joss.04962 is OK
- 10.48550/arXiv.1808.01590 is OK
- 10.1016/j.cpc.2021.108056 is OK
- 10.1103/PhysRevB.108.134102 is OK
- 10.1021/acs.chemmater.3c01628 is OK
- 10.1063/5.0170552 is OK
- 10.1021/acsenergylett.2c02306 is OK
- 10.1126/sciadv.adh8617 is OK
- 10.21105/joss.05974 is OK
- 10.1016/j.commatsci.2017.07.030 is OK
- 10.1038/s41597-020-00638-4 is OK
- 10.1016/j.cpc.2018.01.004 is OK
- 10.1103/PhysRevMaterials.5.123803 is OK
- 10.1088/1674-4926/43/4/042101 is OK
- 10.1103/PhysRevLett.102.016402 is OK
- 10.1103/PhysRevB.89.195205 is OK
- 10.1016/j.cpc.2021.107946 is OK
- 10.1016/j.commatsci.2016.12.040 is OK
- 10.21105/joss.05941 is OK
- 10.1088/0953-8984/16/27/010 is OK
- 10.1016/j.cplett.2017.01.001 is OK
- 10.1103/PhysRevB.54.11169 is OK
- 10.1088/0953-8984/21/39/395502 is OK
- 10.1524/zkri.220.5.567.65075 is OK
- 10.1016/j.cpc.2009.06.022 is OK
- 10.1063/5.0007045 is OK
- 10.48550/arXiv.2402.04434 is OK
MISSING DOIs
- None
INVALID DOIs
- NoneSoftware report:
github.com/AlDanial/cloc v 1.90 T=14.12 s (17.8 files/s, 8247.9 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
XML 5 0 0 56497
Python 25 3422 7325 18596
Jupyter Notebook 10 0 17419 9863
TeX 1 45 0 783
YAML 18 61 32 456
reStructuredText 20 195 305 448
Markdown 4 68 11 383
JSON 158 0 0 367
TOML 1 10 1 109
CSV 7 0 0 42
DOS Batch 1 8 1 26
make 1 4 7 9
-------------------------------------------------------------------------------
SUM: 251 3813 25101 87579
-------------------------------------------------------------------------------Commit count by author:
1093 Sean Kavanagh
55 Seán Kavanagh
27 brlec
22 ireaml
7 Bonan Zhu
6 adair-nicolson
2 savya10
2 “ireaml”
1 Aron Walsh
1 Sabrine Hachmioune
1 sabrine-28Paper file info:
📄 Wordcount for paper.md is 2752
✅ The paper includes a Statement of need section
editorialbot
commented
4 months ago License info:
✅ License found: MIT License (Valid open source OSI approved license)
editorialbot
commented
4 months ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
atimmins7
commented
4 months ago editorialbot
commented
4 months ago I'm sorry human, I don't understand that. You can see what commands I support by typing:
@editorialbot commands
editorialbot
commented
4 months ago I'm sorry human, I don't understand that. You can see what commands I support by typing:
@editorialbot commands
kavanase
commented
3 months ago @editorialbot commands
editorialbot
commented
3 months ago Hello @kavanase, here are the things you can ask me to do:
# List all available commands
@editorialbot commands
# Get a list of all editors's GitHub handles
@editorialbot list editors
# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist
# Set a value for branch
@editorialbot set joss-paper as branch
# Run checks and provide information on the repository and the paper file
@editorialbot check repository
# Check the references of the paper for missing DOIs
@editorialbot check references
# Generates the pdf paper
@editorialbot generate pdf
# Generates a LaTeX preprint file
@editorialbot generate preprint
# Get a link to the complete list of reviewers
@editorialbot list reviewers
@editorialbot generate preprint
editorialbot
commented
3 months ago :page_facing_up: Preprint file created: Find it here in the Artifacts list :page_facing_up:
kavanase
commented
3 months ago @yuan-gist just fyi, I think for the editorialbot to generate your checklist, you need to do the @editorialbot generate my checklist command that you did above, but without any code formatting
yuan-gist
commented
3 months ago yuan-gist
commented
3 months ago Thanks a lot for inviting me for reviewing the doped. Below are my comments:
First-principles calculations play a key role in characterizing and understanding point defects in semiconductors and insulators. Automation codes/toolkits, for example PyCDT and pydefect, greatly facilitate the procedure of setting up defect calculations, parsing the outputs, and obtaining key defect quantities. The new automation toolkits “doped” presents new important features including defect structure analysis, ground-state defect configuration search, and defect thermodynamics estimate. Compared to the previous toolkits, the doped enables much more detailed defect analysis (especially symmetry), which is very valuable for advanced defect studies, like designing defects and constructing descriptors for ML studies. The doped is user-friendly and has clear tutorials. It has already been widely used and attracts lots of attention, which will in turn benefit its future development. I highly recommend publishing the doped on the JOSS journal, and below are some comments that the authors could consider in future development.
pip install doped under conda virtual environmentsnb_path and figs["v_Cd_0" break in generation_tutorial.ipynbwrite_vasp_files(output_path=’./’), which could help users learn to use doped.limits → facets) that are not updated in the tutorials parsing_tutorial.ipynb and thermodynamics_tutorial.ipynbcsv_fname → csv_path in chemical_potentials_tutorial.ipynbbulk_path = "CdTe/CdTe_bulk/vasp_gam/, but FileNotFoundError: [Errno 2] No such file or directory: 'CdTe/CdTe_bulk/vasp_gam/ te_cd_entry = [entry for entry in CdTe_thermo.defect_entries if entry.name == "Te_Cd_1"][0], but IndexError: list index out of rangeRattled with another word that describes more intuitively the POSCAR under this folder.INCAR, should the authors remind the users there are two options: NUPDOWN and MAGMOM?multiplicity from doped is different from the one from pycdt, despite I give both codes the same conventional cell (not primitive cell). For example, I give both codes a CaO conventional cell (NaCl structure, cubic, 8 atoms). doped gives multiplicity=1 for V_Ca, while pycdt gives multiplicity=4. Could the authors help clarify?get_symmetries_and_degeneracies be more independent? These functions are very useful for analyzing old calculations not prepared by doped.That's all my comments.
kavanase
commented
3 months ago Thanks for the diligent review @yuan-gist! I missed this during the last week because I didn't get notified about the edited message, but will get to addressing these points ASAP.
kavanase
commented
3 months ago Here I'm responding point-by-point to track these issues. I'll update as I go, and then bump this thread when complete.
[x] "Using doped to parse calculations not prepared by doped" Was just an issue with the user format, now resolved.
[x] "For installation, I suggest mentioning pip install doped under conda virtual environment" Done! https://github.com/SMTG-Bham/doped/commit/eda6e3d29ac328bc82f23c3f55f7bbfc9a53a7e9; see https://doped.readthedocs.io/en/latest/Installation.html
[x] "The cells about snb_path and figs["v_Cd_0" break in generation_tutorial.ipynb"
Thanks for flagging this! This was due to a recent change in ShakeNBreak which meant that the test files are not downloaded when it is pip installed (as desired), but this should be noted in the tutorial. I've updated to add a comment line to these cells noting that for the demo test data to be used, ShakeNBreak must be git cloned and installed from there (or the path updated to match the local download of the test data). https://github.com/SMTG-Bham/doped/commit/faf6e873d07419d2d71256f1ee9858af260ef43e
[x] "I suggest adding a minimum example of specifying supercell size and shape, for more user control. For example, starting from a silicon conventional cell (in cubic shape), one may want to simply expand it to while keeping the cubic shape. Achieving a minimum image distance is nice, yet sometimes one may want to use a certain supercell shape to do supercell extrapolation. generation_tutorial.ipynb" Good point! The specific example of enforcing a cubic supercell is shown in the full-defects-workflow practice tutorial for MgO here, but I have now added a mini-section demonstrating some supercell generation choices in the advanced analysis tutorial and linked this in the generation tutorial. https://github.com/SMTG-Bham/doped/commit/c3ded5ce56b0a9f46bba69eed31f938b77cb4c03
[x] "For the functions introduced in the tutorials, I suggest (in future update) specifying the arguments, even if they are the default values. For example, write_vasp_files(output_path=’./’), which could help users learn to use doped."
This is a good suggestion. For function calls where there are more than 2 positional arguments, I've now updated to explicitly show the parameter names; https://github.com/SMTG-Bham/doped/commit/c29cd1b9068d733f8259dd9bda975bc6b0e38d7f. For some functions with just one positional argument (e.g. DefectsGenerator(structure, ...) I've left as is, as we also want to avoid the tutorials becoming too verbose (they're already pretty verbose 😅 ).
[x] "There seems to be some renames (limits → facets) that are not updated in the tutorials parsing_tutorial.ipynb and thermodynamics_tutorial.ipynb"
Ah good catch! This was actually updated in a recent commit in develop and are now reflected in the tutorials & repo main branch. https://github.com/SMTG-Bham/doped/commit/f4ccfba72c6e86c4747c52a6ae61d22ab8692dab
[x] "For postprocessing and analysis, I suggest the authors add a short section in the tutorials summarizing what json/csv files will be generated, for which step." Done! Now shown here: https://doped.readthedocs.io/en/latest/Tips.html#serialization-data-provenance-json-csv ; https://github.com/SMTG-Bham/doped/commit/6caee5bfc9667a5e56cdca4898b057201a8114e2
[x] "Please update renames csv_fname → csv_path in chemical_potentials_tutorial.ipynb"
This has also since been updated and there are no usages of csv_fname anywhere in the code/tutorials
[x] "Error in in advanced_analysis_tutorial.ipynb: bulk_path = "CdTe/CdTe_bulk/vasp_gam/, but FileNotFoundError: [Errno 2] No such file or directory: 'CdTe/CdTe_bulk/vasp_gam/" Again, thanks for spotting! This was noted by @adair-nicolson last week and fixed in https://github.com/SMTG-Bham/doped/commit/01bd584ed5b1cabee28b420ba96a732e658428fc
[x] "Error in plotting_customisation_tutorial.ipynb te_cd_entry = [entry for entry in CdTe_thermo.defect_entries if entry.name == "Te_Cd_1"][0], but IndexError: list index out of range"
Thanks for spotting this! There was a typo, should be Te_Cd_+1, fixed now: https://github.com/SMTG-Bham/doped/commit/325eac28878253034a0756697bd18ccfa3c4fbad
[x] "For ShakeNbreak, it might be better to replace the folder name Rattled with another word that describes more intuitively the POSCAR under this folder."
This is something that myself and @ireaml have discussed before. For the particular case where Rattled is used, it's for fully-ionised defect charge states where no specific bond distortions are being applied, only rattling of the atomic positions. We could use Bond_Distortion_0.0% to be consistent with the naming of the bond distortion folders in other directories, but we originally had this and users found it more confusing as it mentioned 'bond distortions' when there are no specific bond distortions being applied in this case, so quickly changed to Rattled. I'm not sure if there's a much more intuitive, concise name for the rattled supercells that we can use, that also justifies changing the naming scheme now when we have a significant user base that are familiar with this architecture and naming scheme.
[x] "For setting the spin multiplicity in INCAR, should the authors remind the users there are two options: NUPDOWN and MAGMOM?" Good point. This was actually on the continued-development to-do list already, done now! https://doped.readthedocs.io/en/latest/Tips.html#spin-polarisation; https://github.com/SMTG-Bham/doped/commit/d52091ceb2649ebdfa381417a1a50b41e2f75547
[x] "For competing phases calculations, how is the k-point density look like in doped? How does it compare with the Materials Project Relaxset k-point density?"
For competing phases, we recommend to determine the converged k-points for each competing phase, as the required densities can differ significantly for different phases (e.g. often 30 kpoints/ų is sufficient for insulators while 1000 kpoints/ų or more are required for certain metals), so choosing one number to use in general is at best inefficient and at worst highly inaccurate. We have built functions in doped to expedite this convergence testing process (e.g. CompetingPhases.convergence_setup()), as is exemplified in the chemical potentials / competing phases tutorial.
When using the VASP input file generation functions for competing phases, it's then recommended to use these converged k-points and an example of doing this is shown in that tutorial. If this is not done, the default k-point densities used are 200 kpoints/ų for metals and 64 kpoints/ų for semiconductors/insulators, which we have found to be reasonable values in most cases. See docstring here.
For comparison, the default pymatgen values are 64 kpoints/ų with MPRelaxSet, 50 kpoints/ų with MPHSERelaxSet and 200 kpoints/ų with MPMetalRelaxSet
[x] "For defect structure generation, the multiplicity from doped is different from the one from pycdt, despite I give both codes the same conventional cell (not primitive cell). For example, I give both codes a CaO conventional cell (NaCl structure, cubic, 8 atoms). doped gives multiplicity=1 for V_Ca, while pycdt gives multiplicity=4. Could the authors help clarify?"
Yes so for the multiplicity in DefectThermodynamics.get_symmetries_and_degeneracies(), the multiplicity refers to the multiplicity of that site in the primitive unit cell (which for CaO is just 2 atoms and so would be 1 for v_Ca). This is done so that the multiplicity is independent of supercell/input-structure, and is noted in the docstring.
Similarly for the Defect objects output by DefectsGenerator, Defect.multiplicity is given with respect to the Defect.structure (noted in docstring), for which the primitive unit cell is used (see docstring):
For the DefectsGenerator output, it gives an output like this: (taking the CdTe example from the generation tutorial):
As noted in this output, the Wyckoff label number (which is a reflection of the site multiplicity) here is given with respect to the conventional cell (as this is the most typical for reporting Wyckoff labels, and to be consistent with the "Conv. Cell Coords" output, so this should give the multiplicity of 4 for v_Ca in the CaO conventional cell.
I have added an additional clarification of this to the thermodynamic analysis tutorial when DefectThermodynamics.get_symmetries_and_degeneracies() is used; https://github.com/SMTG-Bham/doped/commit/393286ad0f80c9ddb0b65910345f301c352ce015
[x] "The paper is clear and very readable. There could be three minor revisions. First, in Figure 2(b), there are "prob." and "probabilities", which could be unified. Second, in Figure 2(b), what is the "ICDS prob."? Third, I suggest the authors cite two recent work related to high-throughput defect calculations: npj Computational Materials (2023) 9:72 ; https://doi.org/10.1038/s41524-023-01015-6 and Yuan et al., Discovery of the Zintl-phosphide BaCd2P2 as a long carrier lifetime and stable solar absorber, Joule (2024), https://doi.org/10.1016/j.joule.2024.02.01715." These have all now been addressed in the minor paper updates; https://github.com/SMTG-Bham/doped/commit/f9aa0cc3d343cac7ce33b0590d45c920654123ab
[x] "The authors mentioned high-throughput compatibility. As far as I checked, there is no tutorial for this. From my high-throughput defect computation experience using the PyCDT (with atomate1) and the pymatgen-defect-analysis (with atomate2), I believe it will be involved to compile the doped with high-throughput computation, as this involves management of job workflow, data, and database. This functionality could be strengthened in future development."
This is a good point, and is something we are currently working on. A tutorial for this is in preparation and is on the continued-development list. A few people have already used doped in combination with AiiDA, and so extension to atomate2 workflows should, for the most part, be relatively painless given the object-oriented basis and use of pymatgen objects when possible (e.g. DictSet derivatives for the VASP input sets).
[x] "In future development, the authors could consider adding a function plotting the defect states (i.e., single-particle defect levels) in the band gap. Such plots are widely seen nowadays."
This is a great suggestion. This was actually on the continuous-development to-do list, and has actually been implemented by @adair-nicolson in the shallow_defects branch. This will be merged to main and included in a doped within the next week. An example of this code in action is available here
[x] "In future development, generating defect complexes could also be a feature. Moreover, designing defects/defect complexes with specific symmetry could be interesting." Yes this is a good suggestion and is also on the continuous-development to-do list (here). There will be some challenges with this (such as properly dealing with symmetry and site equivalency etc.), but I think the current modular code architecture and powerful symmetry/degeneracy functions should allow extension to complex defects without too many headaches. This is one of the primary next targets on the development agenda.
[x] "In future development, could the functions like get_symmetries_and_degeneracies be more independent? These functions are very useful for analyzing old calculations not prepared by doped."
Thanks for the kind words! I agree these are useful functions and the importance of symmetry/degeneracy is often overlooked in defect studies. The point_symmetry_from_defect_entry function in doped.utils.symmetry can be used directly to get the unrelaxed or relaxed site symmetries, where DefectEntry can also be a pymatgen-analysis-defects DefectEntry (as well as a doped DefectEntry, which is a subclass of this).
I have added an additional point_symmetry function now to doped.utils.symmetry (see here) which now offers this same functionality with just structures as input; https://github.com/SMTG-Bham/doped/commit/72b67c965625e4cf101543d461d873a1d9be8eb4
I have also added mini sections to the docs tips page and advanced analysis tutorial about this function and its usage; https://github.com/SMTG-Bham/doped/commit/6b76fe370935387f49c1aef66b455903b6b53bb0
One thing just to note, when testing some of the symmetry determination functions again in more detail here, I noticed that for your CaO defects, the point symmetry determination was quite sensitive to the symprec choice due to the small bond lengths. I have updated the code to improve the robustness of the symmetry determination (now less sensitive to this choice, with a lot of manual checking of the outputs and extensions to the tests for this). So for your case with v_Ca in CaO, these are the correct symmetry outputs (also manually checked):
kavanase
commented
3 months ago I have now addressed all points above ☝️ 😄
yuan-gist
commented
3 months ago I have now addressed all points above ☝️ 😄
Thanks a lot for the efforts! @rkurchin I finished my review and the authors addressed my comments in a very satisfying way!
rkurchin
commented
3 months ago I will speak for @atimmins7 here as I know he's working on a paper deadline for the end of this week and say that he should be able to get to this review next week :)
kavanase
commented
3 months ago @editorialbot generate pdf (Apologies for the spam, just wanted to check the updated PDF output)
editorialbot
commented
3 months ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
atimmins7
commented
3 months ago Apologies for the delay. Will wrap up my review this week.
atimmins7
commented
3 months ago @kavanase @rkurchin
Doped is a thoughtfully designed piece of software for analyzing charged defects. It is well built, well documented, has useful examples, and will certainly be of benefit to the research community. I fully recommend publishing doped in JOSS. Below I have added some suggestions/comments.
*As a note, I primarily use Quantum Espresso and don't have access to VASP. As a result, I could not verify some specific functionality that actively required VASP. Otherwise functionality was tested through the available tutorials and data sets.
get_kumagai_correction arguments (and those for the FNV), but I didn't see this explained in the tutorial. It may be helpful to briefly explain how this error is being determined or what it represents.print(bes) appears to not be aligned with the 'Spin Up' and 'Spin Down' plots. From the printed info, I expected the VBM for spin up to be occupied at the gamma point. Are the titles of the plots perhaps switched?v_Cd_thermo.get_formation_energies? not foundcorrection = F_O_1_entry.get_kumagai_correction(error_tolerance=0.0001) results in error "core.py:280: UserWarning: Estimated error in the Kumagai (eFNV) charge correction for defect F_O_1 is 0.001 eV (i.e. which is greater than the error_tolerance: 0.000 eV)." Given that a very small error tolerance has the digits truncated, is it perhaps worth it to change the error to reflect the number of digits provided by the user?get_quenched_fermi_level_and_concentrations and get_equilibrium_fermi_level do not include keyword argument fermi_dos and running the code blocks with this keyword argument resulted in errors. This issue came up a handful of times in the provided tutorials.kavanase
commented
2 months ago Thanks @atimmins7 for your review and helpful comments! 😃 I've now resolved each of the issues raised, with responses below:
Installation > Requirements. The tutorial states: "doped requires pymatgen>=2022.10.22 and its dependencies." This appears to be out of date?
Ah yes this was missed in a previous update due to changes in pymatgen, now updated to
The doped dependencies are listed in the pyproject.toml file on the GitHub repository.
https://github.com/SMTG-Bham/doped/pull/61/commits/451c954441dc4e1a1c186eb7edc9eaaa8e39c498
Tips and Tricks > Layered / Low Dimensional Materials. Regarding the error "Estimated error in the Kumagai (eFNV) ..." - The meaning of the error is explained in the get_kumagai_correction arguments (and those for the FNV), but I didn't see this explained in the tutorial. It may be helpful to briefly explain how this error is being determined or what it represents.
This is now briefly explained in the get_kumagai/freysoldt_correction docstrings, tips page and parsing tutorials 👍
https://github.com/SMTG-Bham/doped/pull/61/commits/451c954441dc4e1a1c186eb7edc9eaaa8e39c498
Tips and Tricks > Perturbed Host States. Not super clear what information “OrbDiff” is indicating. May be good to specify.
This is now updated in the tips docs page and advanced analysis tutorial 👍 https://github.com/SMTG-Bham/doped/pull/61/commits/451c954441dc4e1a1c186eb7edc9eaaa8e39c498
Tips and Tricks > Perturbed Host States. The info shown with print(bes) appears to not be aligned with the 'Spin Up' and 'Spin Down' plots. From the printed info, I expected the VBM for spin up to be occupied at the gamma point. Are the titles of the plots perhaps switched?
Ah good spot! This was a mislabelling in the code, and has been fixed now (with docs/tutorials images updated). Thanks! https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Defect Calculation Parsing. v_Cd_thermo.get_formation_energies? not found
This was missed in a previous tutorials update (where the object name was changed) – fixed now! https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Defect Calculation Parsing. Output following code correction = F_O_1_entry.get_kumagai_correction(error_tolerance=0.0001) results in error "core.py:280: UserWarning: Estimated error in the Kumagai (eFNV) charge correction for defect F_O_1 is 0.001 eV (i.e. which is greater than the error_tolerance: 0.000 eV)." Given that a very small error tolerance has the digits truncated, is it perhaps worth it to change the error to reflect the number of digits provided by the user?
This has now been updated so that the error and tolerance values in eV are rounded to 3 decimal places (i.e. 1 meV) if the tolerance value is 10 meV or more, otherwise scientific notation with 2 decimal places is used, to avoid excessive rounding for these cases:
https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Defect Thermodynamics and Doping > Dopability Limits. Possible typo in statement "For example here we see that (coordinated by Te anions) is our dominant (lowest-energy) compensating native donor under p-type (Te-rich) conditions, and we can see the doping window of 0.85 eV corresponds to its formation energy at the VBM under Te-rich conditions in the plot above.” I think ‘0.85 eV’ may be a typo. Looking at the table and plot, it looks like it should be ‘0.489’ eV?
Ah yes good spot! The $Cdi^{+2}$ and $V{Cd}^{-2}$ energies were mixed up here, fixed now! https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Defect Thermodynamics and Doping > CdTe: Cd-Rich. Functions get_quenched_fermi_level_and_concentrations and get_equilibrium_fermi_level do not include keyword argument fermi_dos and running the code blocks with this keyword argument resulted in errors. This issue came up a handful of times in the provided tutorials.
Thanks for checking this! This was due to a typo when updating in response to query 5 from reviewer 1 above (https://github.com/SMTG-Bham/doped/commit/c29cd1b9068d733f8259dd9bda975bc6b0e38d7f), fixed now. https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Defect Thermodynamics and Doping > Section 'CdTe: Approximating Temperature-Dependent Band Gap'. My understanding is that the experimental data ('# CdTe gap vs Temp, 2014 study') is added such that we can compare it to the results predicted by doped. Later there is a plot showing how the band gap changes with respect to anneal temp. However, its hard to compare experimental and predicted values as they are split up between plots. Perhaps adjust the experimental plot so that is shows the band gap predicted by doped?
So here the experimental gap is not being predicted by doped, but rather we are just fitting a function to the temperature dependent band gap from experiment, and then using this temperature-dependent band gap in the defect & carrier concentration calculations (as they are affected by the band edge positions, particularly at high temperatures). This section of that tutorial was not overly clear, so I have updated this to better explain what that code is doing & why.
https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Competing Phases > Eigenvalue / Electronic Structure Analysis. It appears that the titles for the plots (up vs down) are flipped wrt to the bes printed data. Same comment as mentioned for Tips & Tricks.
Yep same issue as Q4, fixed now and updated! https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Plot Customization. The chemical potential limit specification of competing phases may be good to briefly explain (e.g. limit="Cd-CdTe")? It might be helpful to briefly clarify what is meant by this setting.
This is now explained in the tutorials 👍 https://github.com/SMTG-Bham/doped/pull/61/commits/9804968ebcc1a97350ab7e98736a3b79c076e7f1
Tutorials > Full Defect Workflow. In section 8.2, you have the following: “Since we have calculated the chemical potentials in the previous section Chemical Potentials, we can just load the results from the JSON file here:” It looks like this is meant to point to chemical potentials section, but the hyperlink points to the doc string.
The link has now been updated to point to the chemical potentials section 👍 https://github.com/SMTG-Bham/doped/pull/61/commits/bbba1fcb605c7e00aa80bd811b781e6470b1b9e5
atimmins7
commented
2 months ago Thanks @kavanase! Updates look good to me.
rkurchin
commented
2 months ago @editorialbot generate post-review checklist
editorialbot
commented
2 months ago I'm sorry human, I don't understand that. You can see what commands I support by typing:
@editorialbot commands
rkurchin
commented
2 months ago @editorialbot set <DOI here> as archive@editorialbot set <version here> as version@editorialbot generate pdf@editorialbot check references and ask author(s) to update as needed@editorialbot recommend-acceptrkurchin
commented
2 months ago @editorialbot check references
rkurchin
commented
2 months ago @editorialbot generate pdf
editorialbot
commented
2 months ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
editorialbot
commented
2 months ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1039/D3TA00532A is OK
- 10.1021/acsaem.2c03466 is OK
- 10.26434/chemrxiv-2024-hm6vh is OK
- 10.1038/s41467-022-32669-3 is OK
- 10.1021/acs.inorgchem.3c01510 is OK
- 10.1021/acs.jpclett.2c02436 is OK
- 10.1039/D2FD00043A is OK
- 10.1021/acsenergylett.1c00380 is OK
- 10.21105/joss.02102 is OK
- 10.1039/D1SC03775G is OK
- 10.1103/PRXEnergy.2.043002 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.1021/acs.chemmater.3c03257 is OK
- 10.1021/acs.jpcc.3c05204 is OK
- 10.1038/s41524-023-00973-1 is OK
- 10.1039/D3CS00432E is OK
- 10.1016/j.matt.2021.06.003 is OK
- 10.21105/joss.04817 is OK
- 10.1088/2516-1075/ace014 is OK
- 10.1016/j.cpc.2018.01.011 is OK
- 10.1016/j.commatsci.2022.111434 is OK
- 10.1039/D3TA02429F is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.21105/joss.04962 is OK
- 10.48550/arXiv.1808.01590 is OK
- 10.1016/j.cpc.2021.108056 is OK
- 10.1103/PhysRevB.108.134102 is OK
- 10.1021/acs.chemmater.3c01628 is OK
- 10.1063/5.0170552 is OK
- 10.1021/acsenergylett.2c02306 is OK
- 10.1126/sciadv.adh8617 is OK
- 10.21105/joss.05974 is OK
- 10.1016/j.commatsci.2017.07.030 is OK
- 10.1038/s41597-020-00638-4 is OK
- 10.1016/j.cpc.2018.01.004 is OK
- 10.1103/PhysRevMaterials.5.123803 is OK
- 10.1088/1674-4926/43/4/042101 is OK
- 10.1103/PhysRevLett.102.016402 is OK
- 10.1103/PhysRevB.89.195205 is OK
- 10.1016/j.cpc.2021.107946 is OK
- 10.1016/j.commatsci.2016.12.040 is OK
- 10.21105/joss.05941 is OK
- 10.1088/0953-8984/16/27/010 is OK
- 10.1016/j.cplett.2017.01.001 is OK
- 10.1103/PhysRevB.54.11169 is OK
- 10.1088/0953-8984/21/39/395502 is OK
- 10.1524/zkri.220.5.567.65075 is OK
- 10.1016/j.cpc.2009.06.022 is OK
- 10.1063/5.0007045 is OK
- 10.48550/arXiv.2402.04434 is OK
- 10.1016/j.joule.2024.02.017 is OK
- 10.1038/s41524-023-01015-6 is OK
MISSING DOIs
- No DOI given, and none found for title: Machine-Learning Structural Reconstructions for Ac...
INVALID DOIs
- None
@editorialbot check references
editorialbot
commented
2 months ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1039/D3TA00532A is OK
- 10.1021/acsaem.2c03466 is OK
- 10.26434/chemrxiv-2024-hm6vh is OK
- 10.1038/s41467-022-32669-3 is OK
- 10.1021/acs.inorgchem.3c01510 is OK
- 10.1021/acs.jpclett.2c02436 is OK
- 10.1039/D2FD00043A is OK
- 10.1021/acsenergylett.1c00380 is OK
- 10.21105/joss.02102 is OK
- 10.1039/D1SC03775G is OK
- 10.1103/PRXEnergy.2.043002 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.1021/acs.chemmater.3c03257 is OK
- 10.1021/acs.jpcc.3c05204 is OK
- 10.1038/s41524-023-00973-1 is OK
- 10.1039/D3CS00432E is OK
- 10.48550/arXiv.2401.12127 is OK
- 10.1016/j.matt.2021.06.003 is OK
- 10.21105/joss.04817 is OK
- 10.1088/2516-1075/ace014 is OK
- 10.1016/j.cpc.2018.01.011 is OK
- 10.1016/j.commatsci.2022.111434 is OK
- 10.1039/D3TA02429F is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.21105/joss.04962 is OK
- 10.48550/arXiv.1808.01590 is OK
- 10.1016/j.cpc.2021.108056 is OK
- 10.1103/PhysRevB.108.134102 is OK
- 10.1021/acs.chemmater.3c01628 is OK
- 10.1063/5.0170552 is OK
- 10.1021/acsenergylett.2c02306 is OK
- 10.1126/sciadv.adh8617 is OK
- 10.21105/joss.05974 is OK
- 10.1016/j.commatsci.2017.07.030 is OK
- 10.1038/s41597-020-00638-4 is OK
- 10.1016/j.cpc.2018.01.004 is OK
- 10.1103/PhysRevMaterials.5.123803 is OK
- 10.1088/1674-4926/43/4/042101 is OK
- 10.1103/PhysRevLett.102.016402 is OK
- 10.1103/PhysRevB.89.195205 is OK
- 10.1016/j.cpc.2021.107946 is OK
- 10.1016/j.commatsci.2016.12.040 is OK
- 10.21105/joss.05941 is OK
- 10.1088/0953-8984/16/27/010 is OK
- 10.1016/j.cplett.2017.01.001 is OK
- 10.1103/PhysRevB.54.11169 is OK
- 10.1088/0953-8984/21/39/395502 is OK
- 10.1524/zkri.220.5.567.65075 is OK
- 10.1016/j.cpc.2009.06.022 is OK
- 10.1063/5.0007045 is OK
- 10.48550/arXiv.2402.04434 is OK
- 10.1016/j.joule.2024.02.017 is OK
- 10.1038/s41524-023-01015-6 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@rkurchin from the editorialbot output above I realised there was a DOI missing for an arXiv reference, fixed now!
rkurchin
commented
2 months ago Great, thanks! I'm gradually (between many meetings 🤪) working through the other checks to finalize this, you can also have a look at the checklist for some of the stuff I'll need from you in terms of a Zenodo/Figshare entry, version tag, etc. and send those along anytime!
kavanase
commented
2 months ago I've made a release of the doped repo following the JOSS review updates, with version tag 2.4.0-JOSS.
This is now archived on Zenodo with matching title name, author list and license at:
10.5281/zenodo.10957359
I've also double-checked the author and affiliation list, so all points on the author to-do list above are ✅
rkurchin
commented
2 months ago Some editorial comments:
\mathrm, \text or equivalent to avoid the weird italicization issues in both the formulae and the subscripts, as in $\text{Cu}_2\text{SiS}_3$ rather than $Cu_2SiS_3$ and also in subscript of vacancies, etc.References:
A question of technical curiosity that is certainly beyond the scope of what's needed for this paper: Have you considered incorporating directional screening in your supercell optimization? If you're using the eFNV scheme that allows for anisotropic screening, it has always seemed to me that you should really optimize over something like the permittivity times the distance rather than just the distance, and I've long wondered how much of a difference that would make in supercell size convergence, etc. If you ever wanted to work together on testing this out, I'd love to chat about it!
rkurchin
commented
2 months ago @editorialbot set 2.4.0-JOSS as version
editorialbot
commented
2 months ago Done! version is now 2.4.0-JOSS
rkurchin
commented
2 months ago @kavanase https://www.doi.org/10.5281/zenodo.10957359 doesn't resolve for me, could you check the copy/paste on the Zenodo link?
kavanase
commented
2 months ago @rkurchin thanks very much for the editorial comments! These have all now been addressed in https://github.com/SMTG-Bham/doped/commit/8da1492ab8723e9e65e399ee74a34b5694e9dd88
As for the question about the screening-dependent supercell generation, yes this is a very good question. I was also asked the same thing by Sam Murphy at an MCC defects workshop in Lancaster in January this year. It's on the development to-do list to implement this (along with k-point-sampling-weighted supercell generation). It should be relatively straightforward to implement with the modular supercell generation, scanning, and optimisation functions in doped, and a user with some moderate python knowledge should be able to implement in their workflow. To be properly added when I have some time!
As for the Zenodo record, yes this is because I added it to the Materials Design Group Zenodo community, which with their new system update requires the community moderator (@aronwalsh) to accept first before it goes live – didn't realise this when I uploaded. Should be live shortly!
kavanase
commented
2 months ago Hi @rkurchin! Just to let you know, the Zenodo record has now been accepted and is live at that link: https://www.doi.org/10.5281/zenodo.10957359 Let me know if you need anything else from our end! 😃
rkurchin
commented
2 months ago @editorialbot 10.5281/zenodo.10957359 as archive
editorialbot
commented
2 months ago I'm sorry human, I don't understand that. You can see what commands I support by typing:
@editorialbot commands
rkurchin
commented
2 months ago @editorialbot set 10.5281/zenodo.10957359 as archive
editorialbot
commented
2 months ago Done! archive is now 10.5281/zenodo.10957359
rkurchin
commented
2 months ago @editorialbot recommend-accept
Submitting author: !--author-handle-->@kavanase<!--end-author-handle-- (Seán R. Kavanagh) Repository: https://github.com/SMTG-Bham/doped Branch with paper.md (empty if default branch): paper Version: 2.4.0-JOSS Editor: !--editor-->@rkurchin<!--end-editor-- Reviewers: @yuan-gist, @atimmins7 Archive: 10.5281/zenodo.10957359
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@yuan-gist & @atimmins7, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @rkurchin know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @atimmins7
📝 Checklist for @yuan-gist