[REVIEW]: DFTK: A Julian approach for simulating electrons in solids #69
Comments
|
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @MasonProtter, @jagot it looks like you're currently assigned to review this paper 🎉. Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post. ⭐ Important ⭐ If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/JuliaCon/proceedings-review) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿 To fix this do the following two things:
For a list of things I can do to help you, just type: For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type: |
|
Failed to discover a |
|
|
Failed to discover a valid open source license. |
|
|
The reviewer already has a pending invite. @jagot please accept the invite by clicking this link: https://github.com/JuliaCon/proceedings-review/invitations |
|
@whedon re-invite @MasonProtter as reviewer |
|
@MasonProtter already has access. |
|
👋 @jagot, please update us on how your review is going (this is an automated reminder). |
|
👋 @MasonProtter, please update us on how your review is going (this is an automated reminder). |
|
@mfherbst I would rewrite
as something like
Also, when I install according to the instructions and try the first tutorial, I get the following warning: ┌ Warning: Package DFTK does not have Plots in its dependencies:
│ - If you have DFTK checked out for development and have
│ added Plots as a dependency but haven't updated your primary
│ environment's manifest file, try `Pkg.resolve()`.
│ - Otherwise you may need to report an issue with DFTK
└ Loading Plots into DFTK from project dependency, future warnings for DFTK are suppressed.@MasonProtter Until JuliaMolSim/DFTK.jl#418 is fixed, you have to do a little bit of monkey patching to run the tutorial: julia> function DFTK.pymatgen_lattice(lattice::AbstractArray)
# Notice: Pymatgen uses rows as lattice vectors, so we unpeel
# our lattice column by column. The default unit in pymatgen is Ångström
mg = pyimport("pymatgen.core.lattice")
bohr_to_A = 1 / austrip(1u"Å")
mg.Lattice([Array(bohr_to_A .* lattice[:, i]) for i in 1:3])
end
julia> function DFTK.pymatgen_structure(model_or_lattice, atoms)
mg = pyimport("pymatgen.core.structure")
pylattice = DFTK.pymatgen_lattice(model_or_lattice)
n_species = sum(length(pos) for (spec, pos) in atoms)
pyspecies = Vector{Int}(undef, n_species)
pypositions = Array{Vector{Float64}}(undef, n_species)
ispec = 1
for (spec, pos) in atoms
for coord in pos
pyspecies[ispec] = spec.Z
pypositions[ispec] = Vector{Float64}(coord)
ispec = ispec + 1
end
end
@assert ispec == n_species + 1
mg.Structure(pylattice, pyspecies, pypositions)
end |
|
@jagot Thanks for the review ... and yes, sorry about the pymatgen issue. That's the second time such a breaking interface change has caused DFTK to stop working properly unfortunately. That's why long-term we definitely want to get rid of it.
Thanks. That's because we load Plots using |
|
Both things should be fixed in the (latest) DFTK 0.2.7 by the way. |
|
@MasonProtter Since you haven't commented yet, let me kindly ping you as a reminder :) |
|
Hi, sorry about this. I will be able to review next week, I've been unexpectedly busy lately, but mid next week I should have time for this. My apologies. |
|
@whedon remind @MasonProtter in 2 weeks |
|
Reminder set for @MasonProtter in 2 weeks |
|
Okay, sorry for all the delays. I really enjoyed reading the paper and exploring the package. This is fantastic work with a slick, powerful user-experience, well written documentation and a ton of features. A few comments: When the checklist asks me if the software source code is available at the respository URL, it leads me to https://github.com/mfherbst/juliacon2020-proceedings-dftk rather than https://github.com/JuliaMolSim/DFTK.jl I would echo @jagot in his suggestion for tweaking the paragraph starting with "For tacking these aspects [...]" Other than these tiny issues, as someone who is familiar with Monte Carlo and DMRG methods, but not so familiar with DFT methods, I find myself wanting at least a couple sentences either in the documentation or (if possible) in the paper which gives me a bit more context how I should expect this general methodolgy to compare to different techniques. Are there any rough rules of thumb I can use for when I should expect DFT to give inaccurate results relative to 'reality'? For instance, I have certainly heard in other places that DFT methods tend to not properly model exchange interactions. I see in the examples section discussion of magnetic systems. I'm curious how well do these calculations agree with experiments, monte carlo, or other benchmarks? I don't need anything super in-depth to give my full approval, but I think it would help me in placing this work in the wider scientific context if there was more comparison to completely different methods, and discussion of potential pitfalls. |
|
I should also say, if you know of a good reference which discusses the limitations and domains of applicability of these sorts of methods, and you feel that the comments in reference is straightforwardly applicable to your work, then that would also satisfy me! |
Thanks for your review and for this guiding question. The problem with answering it, is that it's really tough to do that in just a couple of sentences and at the same time convey a fair picture. We decided to not go into this, simply because this is more of a software paper and not an application paper after all. But I agree, that we don't really point to any literature answering this question either, which we should. I amended the paper appropriately and made a PR to the repo. @MasonProtter let me know if that is not sufficient from your point of view. |
|
@whedon generate pdf |
|
@whedon accept |
|
No archive DOI set. Exiting... |
|
@mfherbst Final thing, can you please archive the current state of the DFTK repository, for example using Zenodo and post the associated DOI here (see https://joss.readthedocs.io/en/latest/editing.html#after-reviewers-recommend-acceptance). Afterwards I can tell whedon to accept the paper! |
|
Well that' done already 😄 ... the most recent commit is a version tag and the zenodo doi is 10.5281/zenodo.4707620. |
|
@whedon set 10.5281/zenodo.4707620 as archive |
|
OK. 10.5281/zenodo.4707620 is the archive. |
|
@whedon accept |
|
|
👋 @JuliaCon/jcon-eics, this paper is ready to be accepted and published. Check final proof 👉 JuliaCon/proceedings-papers#39 If the paper PDF and Crossref deposit XML look good in JuliaCon/proceedings-papers#39, then you can now move forward with accepting the submission by compiling again with the flag |
|
|
@whedon accept deposit=true |
|
I'm sorry @crstnbr, I'm afraid I can't do that. That's something only editor-in-chiefs are allowed to do. |
|
@vchuravy ☝️ |
|
@whedon accept deposit=true |
|
|
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JCON! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
|
@arfon Do you know what's happening here? Is there anything on our side that we can do to fix this? |
|
Hrm, weird. Let me take a look! |
|
@whedon accept deposit=true |
|
|
OK, I think Whedon must have been having a bad day. Things look 👍 to me now. Can you verify? |
|
Yes, to me as well, thanks @arfon 👍. |
|
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets: This is how it will look in your documentation: We need your help! JuliaCon Proceedings is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
|
Thanks for the reviews @jagot and @MasonProtter and for the editing @crstnbr ! |
Submitting author: @mfherbst (Michael F. Herbst)
Repository: https://github.com/mfherbst/juliacon2020-proceedings-dftk.git
Version:
Editor: @crstnbr
Reviewer: @MasonProtter, @jagot
Archive: 10.5281/zenodo.4707620
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
@MasonProtter & @jagot, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @crstnbr 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 ✨
Review checklist for @MasonProtter
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Paper format
paper.texfile include a list of authors with their affiliations?Content
Review checklist for @jagot
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Paper format
paper.texfile include a list of authors with their affiliations?Content
The text was updated successfully, but these errors were encountered: