search

JuliaCon / proceedings-review

6 stars 1 forks source link

[REVIEW]: DFTK: A Julian approach for simulating electrons in solids #69

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@mfherbst<!--end-author-handle-- (Michael F. Herbst) Repository: https://github.com/mfherbst/juliacon2020-proceedings-dftk.git Branch with paper.md (empty if default branch): Version: Editor: !--editor-->@crstnbr<!--end-editor-- Reviewers: @MasonProtter, @jagot Archive:

Status

status

Status badge code:

HTML: <a href="https://proceedings.juliacon.org/papers/c74a09582eb08785dc4883f3c733e7a9"><img src="https://proceedings.juliacon.org/papers/c74a09582eb08785dc4883f3c733e7a9/status.svg"></a>
Markdown: [![status](https://proceedings.juliacon.org/papers/c74a09582eb08785dc4883f3c733e7a9/status.svg)](https://proceedings.juliacon.org/papers/c74a09582eb08785dc4883f3c733e7a9)

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:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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

Content

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

whedon commented 3 years ago

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 :tada:.

:warning: JOSS reduced service mode :warning:

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.

:star: Important :star:

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:

  1. Set yourself as 'Not watching' https://github.com/JuliaCon/proceedings-review:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 3 years ago

Failed to discover a Statement of need section in paper

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.05 s (236.3 files/s, 62851.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
TeX                              8            224            176           2445
Ruby                             1              8              4             45
YAML                             1              1              0             21
Markdown                         1              0              0              2
-------------------------------------------------------------------------------
SUM:                            11            233            180           2513
-------------------------------------------------------------------------------

Statistical information for the repository '8bb4722a9e18d3e1a79961c6' was
gathered on 2021/02/09.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Michael F. Herbst                1            57              0          100.00

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Michael F. Herbst            57          100.0          0.0                7.02
whedon commented 3 years ago

Failed to discover a valid open source license.

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1039/d0fd00048e is OK
- 10.1088/1361-648x/abcbdb is OK
- 10.1038/natrevmats.2015.4 is OK
- 10.1038/ncomms11962 is OK
- 10.1021/jacs.7b02120 is OK
- 10.1038/nmat1752 is OK
- 10.1063/1.3148892 is OK
- 10.1021/acscentsci.8b00229 is OK
- 10.1063/1.5144261 is OK
- 10.1063/5.0005082 is OK
- 10.1103/PhysRevB.54.1703 is OK
- 10.1103/PhysRevLett.77.3865 is OK
- 10.1088/1361-648x/aa680e is OK
- 10.1016/j.commatsci.2012.10.028 is OK

MISSING DOIs

- None

INVALID DOIs

- None
carstenbauer commented 3 years ago

@whedon re-invite @jagot as reviewer

whedon commented 3 years ago

The reviewer already has a pending invite.

@jagot please accept the invite by clicking this link: https://github.com/JuliaCon/proceedings-review/invitations

carstenbauer commented 3 years ago

@whedon re-invite @MasonProtter as reviewer

whedon commented 3 years ago

@masonprotter already has access.

whedon commented 3 years ago

:wave: @jagot, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @MasonProtter, please update us on how your review is going (this is an automated reminder).

jagot commented 3 years ago

@mfherbst I would rewrite

For tackling these aspects multidisciplinary collaboration is essen- tial with mathematicians developing more numerically stable al- gorithms, computer scientists providing high-performance imple- mentations, physicists and chemists designing appropriate models, and application scientists integrating the resulting methods inside a suitable simulation workflow.

as something like

To tackle these aspects, multidisciplinary collaboration with mathematicians developing more numerically stable algorithms, computer scientists providing high-performance implementations, physicists and chemists designing appropriate models, and application scientists integrating the resulting methods inside a suitable simulation workflow is essential.

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 https://github.com/JuliaMolSim/DFTK.jl/issues/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
mfherbst commented 3 years ago

@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.

Also, when I install according to the instructions and try the first tutorial, I get the following warning:

Thanks. That's because we load Plots using Requires, so in some sense it is a "fake-warning". It seems to be fixed with https://github.com/JuliaMolSim/DFTK.jl/pull/419.

mfherbst commented 3 years ago

Both things should be fixed in the (latest) DFTK 0.2.7 by the way.

carstenbauer commented 3 years ago

@MasonProtter Since you haven't commented yet, let me kindly ping you as a reminder :)

MasonProtter commented 3 years ago

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.

carstenbauer commented 3 years ago

@whedon remind @MasonProtter in 2 weeks

whedon commented 3 years ago

Reminder set for @MasonProtter in 2 weeks

MasonProtter commented 3 years ago

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.

MasonProtter commented 3 years ago

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!

mfherbst commented 3 years ago

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?

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.

mfherbst commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

mfherbst commented 3 years ago

With respect to the repo url. I'm not sure there is much I can do, is there? I guess it's because I used a separate repo for the paper.

MasonProtter commented 3 years ago

Yes, this is great.

One other tiny detail: I don't know what the convention is for the JuliaCon proceedings, but in the linked PDF proof, the references are not in the order in which they appear in the paper. I don't have a super strong opinion on it, but it's maybe worth tweaking.

I'm ready to sign off my approval though. :smiley:

mfherbst commented 3 years ago

@MasonProtter Thanks for your feedback :+1:.

the references are not in the order in which they appear in the paper.

I'm not a fan of this either, but it's the same in other JuliaCon papers, so I assume that is the style to be followed.

ranjanan commented 3 years ago

the references are not in the order in which they appear in the paper.

Just to clarify: this is the style of references that we currently use. cc: @vchuravy @matbesancon @crstnbr

carstenbauer commented 3 years ago

@mfherbst Since both reviewers signed off their approval, we're almost done here! (Update: I just noted that @jagot hasn't actually communicated that he's done with his review. @jagot any further comments / criticism from your side?)

The only thing that seems a bit odd to me is that the first heading is "1. Acknowledgments". Can you try to add a heading at the beginning (before the main text), for example "1. Description" or similar, such that acknowledgments becomes the second heading.

(I see that this might push parts of the acknowledgments section to the second page. Perhaps we should move the entire section to the second page. But let's just see how it looks.)

mfherbst commented 3 years ago

I'm not sure it makes a lot of sense to add such a "fake heading". How about making the Acknowledgements a \section* such that the 1. is missing? Would that be ok with the Journal style?

carstenbauer commented 3 years ago

Yes, that would work as well! But please also drop the "2." from "2. References". Thanks.

jagot commented 3 years ago

@crstnbr I'm happy 😊

mfherbst commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

mfherbst commented 3 years ago

@crstnbr That seems to have worked: Acknowledgements and references without section numbers.

carstenbauer commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1039/d0fd00048e is OK
- 10.1088/1361-648x/abcbdb is OK
- 10.1038/natrevmats.2015.4 is OK
- 10.1038/ncomms11962 is OK
- 10.1021/jacs.7b02120 is OK
- 10.1038/nmat1752 is OK
- 10.1063/1.3148892 is OK
- 10.1021/acscentsci.8b00229 is OK
- 10.1063/1.5144261 is OK
- 10.1063/5.0005082 is OK
- 10.1103/PhysRevB.54.1703 is OK
- 10.1103/PhysRevLett.77.3865 is OK
- 10.1088/1361-648x/aa680e is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1063/1.4704546 is OK
- 10.1002/qua.24259 is OK
- 10.1017/cbo9780511805769 is OK

MISSING DOIs

- 10.1137/20m1332864 may be a valid DOI for title: Convergence analysis of direct minimization and self-consistent iterations

INVALID DOIs

- None
carstenbauer commented 3 years ago

@mfherbst Can you confirm that 10.1137/20m1332864 is a valid DOI for the "Convergence analysis of direct minimization and self-consistent iterations" reference? (It looks like it.)

If so, please update the corresponding bib entry. Thanks!

mfherbst commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

mfherbst commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1039/d0fd00048e is OK
- 10.1088/1361-648x/abcbdb is OK
- 10.1137/20M1332864 is OK
- 10.1038/natrevmats.2015.4 is OK
- 10.1038/ncomms11962 is OK
- 10.1021/jacs.7b02120 is OK
- 10.1038/nmat1752 is OK
- 10.1063/1.3148892 is OK
- 10.1021/acscentsci.8b00229 is OK
- 10.1063/1.5144261 is OK
- 10.1063/5.0005082 is OK
- 10.1103/PhysRevB.54.1703 is OK
- 10.1103/PhysRevLett.77.3865 is OK
- 10.1088/1361-648x/aa680e is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1063/1.4704546 is OK
- 10.1002/qua.24259 is OK
- 10.1017/cbo9780511805769 is OK

MISSING DOIs

- None

INVALID DOIs

- None
mfherbst commented 3 years ago

@crstnbr Fixed :smile:

carstenbauer commented 3 years ago

@whedon accept

whedon commented 3 years ago

No archive DOI set. Exiting...

carstenbauer commented 3 years ago

@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!

mfherbst commented 3 years ago

Well that' done already :smile: ... the most recent commit is a version tag and the zenodo doi is 10.5281/zenodo.4707620.

carstenbauer commented 3 years ago

@whedon set 10.5281/zenodo.4707620 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4707620 is the archive.

carstenbauer commented 3 years ago

@whedon accept