Closed editorialbot closed 2 years 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 commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdf
Software report:
github.com/AlDanial/cloc v 1.88 T=0.12 s (407.5 files/s, 86568.9 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 14 522 745 1799
Jupyter Notebook 9 0 4033 1412
Markdown 12 299 0 923
YAML 8 34 126 278
TeX 2 14 0 187
INI 1 11 0 73
HTML 1 0 0 64
Dockerfile 1 15 23 26
make 1 6 8 15
TOML 1 1 3 5
-------------------------------------------------------------------------------
SUM: 50 902 4938 4782
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.48550/ARXIV.1910.00617 is OK
- 10.1038/s41467-020-19964-7 is OK
- 10.48550/ARXIV.1609.02907 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1038/s41524-021-00545-1 is OK
- 10.26434/chemrxiv.11869026.v1 is OK
- 10.48550/ARXIV.1710.10324 is OK
- 10.1103/PhysRevLett.120.145301 is OK
MISSING DOIs
- None
INVALID DOIs
- None
Wordcount for paper.md
is 913
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@dandavies99 and @PeterKraus, let me know if you have any questions about getting your reviews started! Feel free to file issues in the project repository as you do so; please link to this issue for easy tracking.
@rkurchin it's on my todo list for tomorrow.
Alright, this is quite an interesting software package, and it's in a fairly good state. I've made a couple of issues on the project github, I will play around with the software over the next few days. @sgbaird, please let me know (here or in the issues) once you want me to have another look.
@PeterKraus thanks for your feedback! I've addressed each of your comments for the Software paper and the Documentation issues. It's ready for a second look!
(p.s. I wasn't sure if I should be marking the tasks/checkboxes as done or not, so I left them unchecked) EDIT: the ones in the xtal2png repo issues
(@sgbaird to clarify for you, the checklists are to be filled out by the reviewers, so leaving them unchecked was right π )
I'll have a look on Monday. Have a nice weekend!
Hi @sgbaird, this software is looking great. @PeterKraus - thanks for getting there first and carrying out such a thorough review! You've covered almost all of the main points I spotted already. I'll update comments in this thread and open specific issues as I go through the checklist. A few initial minor points from me (apologies if these are already being addressed in separate issues):
conda create -n xtal2png -c conda-forge xtal2png m3gnet
at the top and conda env create -n xtal2png -c conda-forge xtal2png
in the Installation section. The inclusion of m3gnet
results in a very long (possibly infinite at the time of writing π) environment solving time. I understand m3gnet is optional, which is explained, but it might be worth not including this in the first installation instruction to save the user some frustration.XtalConverter
is in xtal2png.core
now, not in xtal2png
. Similarly example_structures
is now in xtal2png.utils.data
. The paper is one of the longer JOSS papers I've come across, certainly within chemistry, but I like the level of detail so personally think this is fine. It hits all the criteria above in the checklist.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Looks really great. I'll give the paper another read, and I have a few crystal structures that I'd like to test out, but apart from that I'm more than happy.
From my side, once the minor points I've mentioned above are addressed I will tick off Installation and Functionality. Other than that, I'm also happy and look forward to seeing this package in action - I'm sure it'll be very widely used.
Alright, I have made two more issues, but they are not a blocker for me accepting. After you address the comments in https://github.com/sparks-baird/xtal2png/issues/144, feel free to close the issue. The documentation issue, https://github.com/sparks-baird/xtal2png/issues/146, can be closed already.
@rkurchin, do I need to do anything with the bot to recommend accept?
Thanks @PeterKraus! And nope, you just need to finish your checklist. Once @dandavies99 is satisfied, I'll take care of the final steps :)
@sgbaird, have you had a chance to implement @dandavies99's suggestions above? I think that's the only remaining blocker here...
I just got back from 2 weeks of international travel, and during that @kjappelbaum took the initiative to address @dandavies99 's comments. I looked over and merged the changes as well.
Looking good. Just one query which I've commented on this issue.
Great all good after getting to the bottom of that, happy to approve this π
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Thanks everyone! @sgbaird, I'll do an editorial pass over the manuscript and send any comments shortly. In the meantime, the next steps for you are:
Conceptual comment:
In your Statement of Need, you compare crystal graph convolution (e.g. cgcnn.py
to what you've done here, but I would argue there's a meaningful conceptual distinction there, in that CGCNN's are their own model architecture taking in a crystal graph as input. What you've done is create a new kind of data representation as images (which one could think of as graphs with a very regular local adjacency structure), which allows invocation of a whole bunch of image-based architectures without needing to generalize them to arbitrary graphs, the way GCNN's are a generalization of CNN's. I would consider whether another example/framing might help clarify this...
(Curious also for the reviewers' takes on this critique if they care to chime in, @sgbaird and @dandavies99)
Some editorial suggestions:
P.S. I think this is a pretty nifty idea and may be trying it out in some of my own work!
Conceptual comment: In your Statement of Need, you compare crystal graph convolution (e.g.
cgcnn.py
to what you've done here, but I would argue there's a meaningful conceptual distinction there, in that CGCNN's are their own model architecture taking in a crystal graph as input. What you've done is create a new kind of data representation as images (which one could think of as graphs with a very regular local adjacency structure), which allows invocation of a whole bunch of image-based architectures without needing to generalize them to arbitrary graphs, the way GCNN's are a generalization of CNN's. I would consider whether another example/framing might help clarify this... (Curious also for the reviewers' takes on this critique if they care to chime in, @sgbaird and @dandavies99)
That's a good point. CGCNN was a generalization of a CNN and affected how the data was represented. This is more about screening models using a new data representation. E.g. try xtal2png
with imagen-pytorch
, a GAN, and a VAE, and see which one does better, then decide whether to spend more time making a generalized data representation + model combination for the one(s) that perform best. Hence the screening.
Maybe I could use https://github.com/aspuru-guzik-group/chemical_vae as an example, which I think was the first implementation of VAEs to molecules. At least it was probably the first widely publicized/popular one. VAEs introduced in 2013, Chemical VAE implemented in ~2018.
I could also use CDVAE as an example which was first online (arXiv) Oct 2021, introduced in 2015.
Your coment about it being "a graph with a very regular local adjacency structure" reminded me of this twitter comment:
The interesting part to me is that pixels carry different info (features vs positions). Image convolutions/kernels are funky in this scenario. Feels like a messier graph CNN where edges and node features are mixed up⦠curious how it goes!
Some editorial suggestions:
- line 10: "natural language" -> "natural language processing"
- 11: capitalize and add a comma after Transformers (also capitalize in other places it shows up, e.g. line 23)
- 15: delete "respectively"
- 22: be consistent with citation formatting, put Vaswani et al. inside citation
- 23: semicolon should be a comma (second clause could not be a sentence on its own)
- lines 31 and 33: something weird is happening with the double-parentheses for citation
- 60: either add a comma after before "which" or change it to "that"
Great, will get these implemented.
P.S. I think this is a pretty nifty idea and may be trying it out in some of my own work!
Glad to hear!
@rkurchin thanks for the great feedback. I addressed your comments. Lmk if this looks OK.
Looking back over the manuscript, I realized I probably need to add to the acknowledgments section (https://github.com/sparks-baird/xtal2png/issues/193) and will post the final version and DOI after that.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
LGTM! (modulo version/DOI)
Version: v0.9.2
DOI: 10.5281/zenodo.6941615
@editorialbot set 0.9.2 as version
Done! version is now 0.9.2
@editorialbot set 10.5281/zenodo.6941615 as archive
Done! Archive is now 10.5281/zenodo.6941615
@editorialbot check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acscentsci.7b00572 is OK
- 10.48550/ARXIV.1910.00617 is OK
- 10.1038/s41467-020-19964-7 is OK
- 10.48550/ARXIV.1609.02907 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1016/j.matt.2021.11.032 is OK
- 10.48550/ARXIV.2204.00056 is OK
- 10.1038/s41524-021-00545-1 is OK
- 10.26434/chemrxiv.11869026.v1 is OK
- 10.1021/ci00057a005 is OK
- 10.48550/ARXIV.1710.10324 is OK
- 10.1103/PhysRevLett.120.145301 is OK
MISSING DOIs
- 10.1145/3528233.3530757 may be a valid DOI for title: Palette: Image-to-Image Diffusion Models
INVALID DOIs
- 10.48550/ARXIV.1610.02415v1 is INVALID
@sgbaird almost there! Can you just fix up those couple of references? Then we should be good to accept!
@rkurchin and release a new version?
References are fixed. It looks like it didn't like the v1
for arXiv, which is interesting since that implies the "object" associated with the DOI can still have content appended to it. Maybe that's normal though since I think Zenodo supports something similar with the latest version DOI.
Either way, working on releasing v0.9.3
.
Version: v0.9.3
DOI: 10.5281/zenodo.6941657
@editorialbot check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.48550/ARXIV.1610.02415 is OK
- 10.1021/acscentsci.7b00572 is OK
- 10.48550/ARXIV.1910.00617 is OK
- 10.1038/s41467-020-19964-7 is OK
- 10.48550/ARXIV.1609.02907 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1016/j.matt.2021.11.032 is OK
- 10.1145/3528233.3530757 is OK
- 10.48550/ARXIV.2204.00056 is OK
- 10.1038/s41524-021-00545-1 is OK
- 10.26434/chemrxiv.11869026.v1 is OK
- 10.1021/ci00057a005 is OK
- 10.48550/ARXIV.1710.10324 is OK
- 10.1103/PhysRevLett.120.145301 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@editorialbot set 10.5281/zenodo.6941657 as archive
Done! Archive is now 10.5281/zenodo.6941657
@editorialbot set 0.9.3 as version
Done! version is now 0.9.3
Submitting author: !--author-handle-->@sgbaird<!--end-author-handle-- (Sterling Baird) Repository: https://github.com/sparks-baird/xtal2png Branch with paper.md (empty if default branch): main Version: 0.9.4 Editor: !--editor-->@rkurchin<!--end-editor-- Reviewers: @dandavies99, @PeterKraus Archive: 10.5281/zenodo.6941663
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
@dandavies99 & @PeterKraus, 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 @PeterKraus
π Checklist for @dandavies99