Open editorialbot opened 2 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 commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdf
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.26434/chemrxiv-2023-v969f-v3 is OK
- 10.1021/acs.jctc.8b00640 is OK
- 10.26434/chemrxiv.8304578.v1 is OK
MISSING DOIs
- No DOI given, and none found for title: Sage-2.1.0
INVALID DOIs
- None
Software report:
github.com/AlDanial/cloc v 1.90 T=0.15 s (1043.7 files/s, 178972.5 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 60 4771 4534 15410
reStructuredText 89 426 880 503
Markdown 5 67 0 270
TeX 1 3 0 36
make 1 4 7 9
Bourne Shell 1 0 0 3
-------------------------------------------------------------------------------
SUM: 157 5271 5421 16231
-------------------------------------------------------------------------------
Commit count by author:
95 Trevor Gokey
1 trevorgokey
Paper file info:
📄 Wordcount for paper.md
is 1242
✅ The paper includes a Statement of need
section
License info:
✅ License found: MIT License
(Valid open source OSI approved license)
I assumed JOSS reviewers would install the branch considered for submission, in this case joss-manuscript. In this case, the clone command should be followed by git checkout joss-manuscript before the install command.
@trevorgokey The JOSS review is usually based on the main
/ master
branch, with any additional joss-manuscript
branch usually containing the JOSS paper. If you want joss-manuscript
to be reviewed that is fine also, and then we'd typically expect it to be merged into main
once review is complete.
(Re-pasting from the pre-review thread so not lost as we move across to the review thread)
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
The JOSS review is usually based on the main / master branch, with any additional joss-manuscript branch usually containing the JOSS paper. If you want joss-manuscript to be reviewed that is fine also, and then we'd typically expect it to be merged into main once review is complete.
@lucydot thanks for the explanation. I am fine with merging the paper into main after review. I am still actively contributing to the codebase and pushing to main, so I intended the joss-manuscript
branch to be a frozen, versioned codebase to review. I've already made some commits to main
since submitting this, but while this is under review I'll develop on a different branch to avoid unintentionally breaking something.
@trevorgokey I can install through the method in the README.md page but cannot run the examples in the RTD. Also, the RTD installation page says the packages can be directly installed through pip. Please make them consistent and ensure the examples can run.
Hi @trevorgokey - a nudge in case you missed the notification from message above ☝️ . This appears important to address for review process.
@Ruibin-Liu - can you point towards what the problem was with the examples in RTD. Is there a particular error message you are getting? This may help @trevorgokey address the issue.
The error is:
...besmarts-git/besmarts-core/python/setup.py", line 5, in
Hi, thanks for indicating the issue. I fixed that error in the pre-review stage when @Ruibin-Liu first reported issues with installing. I just installed the code using a fresh environment and I did not have any issue.
I can install through the method in the README.md page but cannot run the examples in the RTD
I am noticing that some of the RTD is out of date and I will need to go over it and make it consisent with the package. I worked on the examples in besmarts/besmarts-core/python/examples
but did not update the docs examples. Thank you @Ruibin-Liu for pointing this out.
I just installed the code using a fresh environment and I did not have any issue.
Could this confusion result from using different branches? @Ruibin-Liu - I believe the branch to review is joss-manuscript
- can you confirm that is correct @trevorgokey ?
I don't have issue to install manually now but I don't think the RTD claim for pip installation method is correct. Specifically, this line pip install besmarts-hierarchy besmarts-splitter besmarts-resolve
is incorrect in that none of them is in PyPI.
Other issues in the RTD examples I have checked.
from besmarts.codecs import native
doesn't work in the Graphs section.for indices in graphs.graph_dihedrals(g):
print(indices)
for indices in graphs.graph_impropers(g):
print(indices)
print(geometry.bond(idx[::-1]))
incorrect because idx
is int
There are more in the RTD doc. I stopped checking. Please make sure those examples can run without any issue.
Thanks for update @Ruibin-Liu - @trevorgokey it sounds like there is some more updates to make to the documentation so examples can be ran locally. If you could update here with expected timescales for fixing that would be useful -
I'm out for a conference right now and will work on the documentation immediately after it ends. I should have it done in the next few days.
On Wed, May 15, 2024, 10:17 AM Lucy Whalley @.***> wrote:
Thanks for update @Ruibin-Liu https://github.com/Ruibin-Liu - @trevorgokey https://github.com/trevorgokey it sounds like there is some more updates to make to the documentation so examples can be ran locally. If you could update here with expected timescales for fixing that would be useful -
— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/6653#issuecomment-2111866271, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL7KZRVA7LZGWH2ITQ7YPJLZCMKZ7AVCNFSM6AAAAABGSVONAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJRHA3DMMRXGE . You are receiving this because you were mentioned.Message ID: @.***>
Hi @trevorgokey , I've opened up an issue trevorgokey/besmarts#1 which contains my detailed breakdown of my current problems that I have found in this repository. A few may overlap @Ruibin-Liu 's comments.
Here is a checklist summary of what needs to be done before I can complete the review on my end, but also look at the issue for full details:
pytest
runs with no issues.Thanks for going through the documentation @Ruibin-Liu and @AntObi. I've since given the RTD documentation an overhaul and should be all working as far as I can tell, including installation, tests, and examples. Also, I've made the main
and joss-manuscript
branches the same just to reduce any confusion.
I'll continue to work on code quality such as docstrings; lots of the newer stuff is in flux and so I haven't quite fixed their behavior. Unfortunately, a lof of the newer stuff is also the most complicated so I apologize if the interesting bits of the code have been opaque. I'll continue working on the second checklist provided by @AntObi.
Thanks for the update @trevorgokey ; what do you think a reasonable estimate for timescale until points are addressed? Note that we do not require full test or documentation coverage (although of course this is a good point to aim for), rather we require the core functionality is sufficiently tested and documented.
In a week's time I should be able to give everything a first pass. My plan is to go through all of the examples and tests and make sure that all forward-facing code (examples and everything in RTD) are all documented. I'll check back in once this is complete.
A quick note here to say I will be on Annual Leave for the next week, will be back on the 24th June.
@trevorgokey; an update on how the amendments are going and timeframe would be great.
Submitting author: !--author-handle-->@trevorgokey<!--end-author-handle-- (Trevor Gokey) Repository: https://github.com/trevorgokey/besmarts Branch with paper.md (empty if default branch): joss-manuscript Version: 0.1.0 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @AntObi, @Sulstice, @Ruibin-Liu Archive: Pending
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
@AntObi & @Sulstice & @Ruibin-Liu, 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 @lucydot 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 @AntObi
📝 Checklist for @Ruibin-Liu