search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
720 stars 38 forks source link

[REVIEW]: LearnSAT: A SAT Solver for Education #639

Closed whedon closed 6 years ago

whedon commented 6 years ago

Submitting author: @motib (Mordechai Ben-Ari) Repository: https://github.com/motib/LearnSAT Version: V2.0 Editor: @danielskatz Reviewer: @audemard, @biotomas Archive: 10.5281/zenodo.1230536

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/7bb33dc5b9f0c455eed04642dc710d6b"><img src="http://joss.theoj.org/papers/7bb33dc5b9f0c455eed04642dc710d6b/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/7bb33dc5b9f0c455eed04642dc710d6b/status.svg)](http://joss.theoj.org/papers/7bb33dc5b9f0c455eed04642dc710d6b)

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

Reviewer1 instructions & questions

@audemard, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @danielskatz know.

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Reviewer2 instructions & questions

@biotomas, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @danielskatz know.

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @audemard it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) 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/openjournals/joss-reviews:

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
whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

danielskatz commented 6 years ago

@arfon, please invite @biotomas so that he can do this review

danielskatz commented 6 years ago

@audemard & @biotomas - this review is now open. See instructions above.

Please use this issue to track any questions/concerns you have, even if you actually create new issues on focused details in https://github.com/motib/LearnSAT

Any questions on the review, please ask.

motib commented 6 years ago

@audemard , @biotomas,

Thank you for agreeing to review my submission.

I am leaving today for a 12-day vacation (really, I'm not taking a laptop!). While I will be able to respond to general queries, addressing detailed questions about the software will have to wait until the beginning of April.

danielskatz commented 6 years ago

@arfon, please invite @biotomas so that he can do this review

arfon commented 6 years ago

@arfon, please invite @biotomas so that he can do this review

Done.

danielskatz commented 6 years ago

@audemard @biotomas - I'm hoping that you both can start your reviews soon.

In case it's not clear, your job is basically to go through the checklist items, making sure that all can be checked. To do this, you will also have to examine the paper and the contents of the code repository.

If you can check off an item, please do. If you cannot, please either discuss it here if it is brief, or open an issue in the code repository and put a comment here pointing to that issue.

Thanks.

danielskatz commented 6 years ago

@audemard @biotomas - it looks like both have you have started reviews, and checked off some items, but there are also some unchecked, and I don't see any comments from you here or issues in https://github.com/motib/LearnSAT, so I'm unsure what the status of your reviews are.

If there are items you cannot check off, please explain them here.

biotomas commented 6 years ago

Hi, so I left two checkboxes unchecked because I found no DOIs in references and also no community guidelines.

danielskatz commented 6 years ago

@motib - In response to the comments by @biotomas:

  1. It looks like the handbook chapters have DOIs (http://ebooks.iospress.nl/volume/handbook-of-satisfiability) so you should be able to add at least that one DOI. I don't think there are DOIs for the two books, but if they have similar identifiers, such as ISBNs, please add them.

  2. While looking at https://github.com/motib/LearnSAT/blob/master/learnsat.bib, I also notice you have quite a few entries in this file that are not actually cited in the paper.md file and thus are not references. I don't know if you want to cite any more of the bib entries.

  3. I also see that there is no References header in the paper.md file. I've created https://github.com/motib/LearnSAT/pull/1 to fix this.

  4. Please also address the lack of community guidelines.

motib commented 6 years ago

My thanks to @biotomas for his comments.

  1. I have chased down DOIs and ISBNs for all the references and will update the documents.

  2. The file "learnsat.bib" contains the bibtex entries for all three of the documents included in the archive as well as those in paper.md. Since latex is smart enough to extract only the references cited for each document, I don't see this as a problem. To the contrary, if I had four separate files, there would be a risk of inconsistency.

  3. I am rather new to git. Would it be correct for me simply to click "merge"?

  4. I'm sorry for not noticing this. The submission instructions don't specify where this information should be included. In the "readme"? In a separate file? If so, is there a convention for naming it?

@danielskatz: please clarify points 3 and 4 and I'll update the submission.

danielskatz commented 6 years ago

@motib -

  1. I am rather new to git. Would it be correct for me simply to click "merge"?

yes, though of course you should first check the changed file(s) to make sure you want to merge it.

  1. I'm sorry for not noticing this. The submission instructions don't specify where this information should be included. In the "readme"? In a separate file? If so, is there a convention for naming it?

This is commonly in both the readme and the documentation, but there is no requirement - it should be where you think it will be the most useful to potential contributors.

motib commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

motib commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

motib commented 6 years ago

I have made the modifications, but the isbns/dois do not appear on the generated pdf of the "paper". Possibly, it is not including the package \url that I use to format the dois. If so, should I remove \url or is there someway to add this package?

Also: @danielskatz I just noticed that you forked the project, but I made the modifications to the master branch. Is that OK?

danielskatz commented 6 years ago

Also: @danielskatz I just noticed that you forked the project, but I made the modifications to the master branch. Is that OK?

It looks like you made the equivalent change to what was in my branch without merging my branch's PR, which is fine, so I've now deleted the branch and closed the PR.

danielskatz commented 6 years ago

I have made the modifications, but the isbns/dois do not appear on the generated pdf of the "paper". Possibly, it is not including the package \url that I use to format the dois. If so, should I remove \url or is there someway to add this package?

@arfon, can you help with this? I would have expected the note data to be pulled from the .bib file into the references. Perhaps you can suggest how this should be done?

danielskatz commented 6 years ago

Also, @motib - I see that you have included a community-guidelines.md file, but can I suggest that you add this to the README instead or in addition? And potentially in or around section 1 or 2 of https://github.com/motib/LearnSAT/blob/master/docs/learnsat-ug.pdf too?

danielskatz commented 6 years ago

@arfon - ping... re: 2 comments up :)

arfon commented 6 years ago

@arfon, can you help with this? I would have expected the note data to be pulled from the .bib file into the references. Perhaps you can suggest how this should be done?

Sure. I think these should be listed as doi fields as well as url. So for this line change it to: 

doi={10.1145/3107239},
url={http://dx.doi.org/10.1145/3107239}
danielskatz commented 6 years ago

And what do you suggest for the ISBNs for the books? I would have expected the note field to work.

motib commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

arfon commented 6 years ago

And what do you suggest for the ISBNs for the books? I would have expected the note field to work.

According to this post the note field should work https://tex.stackexchange.com/questions/52040/bibtex-show-isbn-number. I'm not sure what bibliography style we're using for JOSS but it's clearly not including note fields by default...

motib commented 6 years ago

@arfon: On stackexchange I found a simple way to include DOIs: change the bib style to "plainurl" and include the "hyperref" package. "hyperref" additionally creates internal links in the PDF. Now the documents include "doi" fields with no "http" but the fields are clickable links. On the whedon-generated pdf, the "doi" appears as a link with "http".

There is no simple way include isbn's (without major surgery to the source files that I don't want to do and don't see a need for). The suggestion is to use "note" since books are easy to search for on google or amazon. This works OK for the documents, but not for the whedon-generated pdf. Since that is not under my control, I can't fix it ....

As per @danielskatz's suggestion, I moved the community guidelines to the readme file and referred to it in the User Guide. I took the opportunity to modify the UG by deleting the instruction to download from my personal website; instead, I give links to the JOSS article and to the GitHub repo.

danielskatz commented 6 years ago

@arfon

According to this post the note field should work https://tex.stackexchange.com/questions/52040/bibtex-show-isbn-number. I'm not sure what bibliography style we're using for JOSS but it's clearly not including note fields by default...

Is this something you think we can fix, or should we just accept that we can't do this for now?

danielskatz commented 6 years ago

@biotomas - Please check to see if you are ok with the updates that have been made, and can check off the remaining items

danielskatz commented 6 years ago

@audemard - please continue your review, or let us know what issues are blocking you.

arfon commented 6 years ago

Is this something you think we can fix, or should we just accept that we can't do this for now?

I think it's worth opening an issue to track this bug but I don't think this can be fixed quickly.

danielskatz commented 6 years ago

ok, I opened https://github.com/openjournals/joss/issues/392

biotomas commented 6 years ago

I checked off all the items

danielskatz commented 6 years ago

:wave: @audemard - please continue your review, or let us know what issues are blocking you.

audemard commented 6 years ago

Sorry for the delay. It is done now.

arfon commented 6 years ago

Sorry for the delay. It is done now.

@audemard - thanks for completing your review. We'll close out this issue once we've done the final processing of this submission.

danielskatz commented 6 years ago

Great! :wave: arfon - can we move this to acceptance now?

arfon commented 6 years ago

@motib - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

motib commented 6 years ago

@arfon: I have never heard of these before so I have a couple of questions. I logged into Zenodo (with my GitHub account).

  1. It wants me to upload files. Should I update individual files or could I just download a zip of the GitHub repository and upload that? I would prefer the latter since the directories on my computer have additional stuff like my local backups that I don't want to upload.

  2. It asks "Did your publisher already assign a DOI to your upload?" Does that mean that I should give them the JOSSS DOI or should I let Zenodo assign one? In the latter case, should I add the JOSS DOI to the "related identifier" field? Thanks....

arfon commented 6 years ago

It wants me to upload files. Should I update individual files or could I just download a zip of the GitHub repository and upload that? I would prefer the latter since the directories on my computer have additional stuff like my local backups that I don't want to upload.

@motib - I'd suggest following this guide - it should be pretty simple: https://guides.github.com/activities/citable-code/

It asks "Did your publisher already assign a DOI to your upload?" Does that mean that I should give them the JOSSS DOI or should I let Zenodo assign one? In the latter case, should I add the JOSS DOI to the "related identifier" field?

We don't assign a DOI for your upload. Let Zenodo assign one (and paste it in this thread).

In the latter case, should I add the JOSS DOI to the "related identifier" field?

You can do this. Your JOSS DOI will be https://doi.org/10.21105/joss.00639

motib commented 6 years ago

I think I did everything OK! The Zenodo DOI is https://doi.org/10.5281/zenodo.1230536

danielskatz commented 6 years ago

@whedon set https://doi.org/10.5281/zenodo.1230536 as repository

danielskatz commented 6 years ago

@whedon set 10.5281/zenodo.1230536 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1230536 is the archive.

danielskatz commented 6 years ago

👋 @arfon - back to you to finish this one