search

openjournals / joss-reviews

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

[REVIEW]: ScenTrees.jl: A Julia Library For Generating Scenario Trees and Scenario Lattices for Multistage Stochastic Optimization #1912

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @kirui93 (Kipngeno ) Repository: https://github.com/kirui93/ScenTrees.jl Version: v0.2.2 Editor: @melissawm Reviewer: @juliohm, @matbesancon Archive: 10.5281/zenodo.3672205

Status

status

Status badge code:

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

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

@juliohm & @matbesancon, 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 @melissawm know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @juliohm

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @matbesancon

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @juliohm, @matbesancon it looks like you're currently assigned to review 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

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

@whedon generate pdf
whedon commented 4 years ago 
Attempting to check references...
whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago 

OK DOIs

- 10.1287/mnsc.47.2.295.9834 is OK
- 10.1007/s10479-015-1994-2 is OK
- 10.1007/s101070000202 is OK
- 10.1137/080718401 is OK
- 10.1137/110825054 is OK
- 10.1007/s10589-015-9758-0 is OK
- 10.1137/15M1043376 is OK
- 10.1137/141000671 is OK
- 10.1016/j.ejor.2016.11.028 is OK
- https://doi.org/10.1016/0167-7152(92)90006-Q is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

kirui93 commented 4 years ago

@whedon generate pdf

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

:point_right: Check article proof :page_facing_up: :point_left:

kirui93 commented 4 years ago

@whedon generate pdf

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

:point_right: Check article proof :page_facing_up: :point_left:

melissawm commented 4 years ago

Hi all, just checking in to see if there's anything I can help you with.

matbesancon commented 4 years ago

@melissawm a bunch of issues were opened, some being "nice-to-have" improvements on the package, but some being fairly necessary to navigate through the package and understand what is going on, how to use it & for which classes of problems. My opinion is that there is a fair bit of polishing still to do but I'll let the author voice in

matbesancon commented 4 years ago

cc @kirui93

kirui93 commented 4 years ago

@matbesancon Thank you so much for mentioning that. I think I have implemented all the review comments that you had stated and that has enabled us to bump up the package. If you update to the new version, you will see that almost all the details are incorporated. What we are remaining with only is to change to use Plots.jl instead of PyPlot.jl which we will do it in some time to come as for now PyPlot.jl works well.

kirui93 commented 4 years ago

@matbesancon I also did polish the paper and I think it is much of straightforward and to the point now.

kirui93 commented 4 years ago

@matbesancon @juliohm I really believe that everything is now up to standard of the package we wanted to create. Your insightful review comments have been of great help.

juliohm commented 4 years ago

@kirui93 please comment on each open issue to explain how you addressed them. There are many open issues still.

kirui93 commented 4 years ago

@juliohm I think most of the issues I have dealt with. So I will comment and close them if it is up to my side.

kirui93 commented 4 years ago

@matbesancon @juliohm I did comment on the issues created and closed most of them as I had already dealt with them.

juliohm commented 4 years ago

@kirui93 you didn't address many issues raised in the software paper for example. You also didn't mention how you addressed each issue on GitHub before closing them.

Please report on each issue above how you addressed the review comments, otherwise it is difficult to approve the paper for publication in JOSS.

kirui93 commented 4 years ago

@kirui93 you didn't address many issues raised in the software paper for example. You also didn't mention how you addressed each issue on GitHub before closing them.

Please report on each issue above how you addressed the review comments, otherwise it is difficult to approve the paper for publication in JOSS.

  1. General comments about the documentation - Initially, I had my docstrings placed in a wrong manner and with the help of @matbesancon I was able to rewrite the docstrings in the correct manner. And also @juliohm had mentioned briefly about the usage of "library" instead of "package" in most parts of the documentation, which I addressed accordingly.

  2. @matbesancon indicated about the title and the project set up. He was suggesting to name the package from "ScenTrees.jl" to something like "ScenarioTrees.jl". Initially, we had a lot of suggestions for the name of the package but we decided to go with "ScenTrees.jl" as it is unique and also in a way describes to our goal.

  3. On naming and conventions, @matbesancon indicated about the function Children{Vector{Int64}} which looked like a constructor. As indicated, this function is an internal function of Tree and its main purpose is to reduce the number of times we are going to use the function in the stochastic approximation process. We access the children of the nodes each time in stochastic approximation iteration and hence saving the children as a vector at first with the "Chidren" function provides an easier and less expensive manner.

  4. Another issue concerned incorporating AbstractTrees.jl. Though it was not a major issue, but as I had indicated, this package seems it is no longer maintained. And I couldn't find the documentation to detail its usage.

  5. One of the most important issue that @matbesancon had stated was every function which performs a random sampling should take in parameter the Random Number Generator (which is usually a subtype of Random.AbstractRNG). I found this as a good first issue as as he had indicated making function runs deterministic makes it really important. With this I used the package Random.jl. As stated in their documentation, random number generation in Julia uses the Mersenne Twister library via MersenneTwister objects and thus I employed this for every function that involves random number generation.

  6. There was quite a mislead regarding the Wasserstein distance and we thank @matbesancon for noting this out. In our code, we had used the term "WassersteinDistance" which was inappropriate. I stated in the issue that this was supposed to be the multistage distance and is duly corrected in the code. There is also the parameter r which is used for multistage distance and is also corrected well in all the codes.

  7. The final issue that @juliohm opened was about improvements for the software paper. All the corrections he suggested are duly represented in the paper. The current version of the paper has all the corrections implemented.

Thank you so much @matbesancon and @juliohm for your patience as well as insightful comments about the software paper as well as the package in general.

juliohm commented 4 years ago

@kirui93 can you please add these comments to the issues in your project repo instead? It is quite hard to follow any text here without context. Please understand that we have limited time to review.

I will try to read the issues again in the following weeks.

Thank you.

juliohm commented 4 years ago

@whedon generate pdf

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

:point_right: Check article proof :page_facing_up: :point_left:

kirui93 commented 4 years ago

@juliohm @matbesancon Thank you so much for your patience. I have re-commented again on the important issues and re-opened them.

kirui93 commented 4 years ago

Some other issues are general comments that were just suggestions for improving the documentation as well as docstrings which I did implement immediately on statement.

kirui93 commented 4 years ago

@whedon generate pdf

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

:point_right: Check article proof :page_facing_up: :point_left:

melissawm commented 4 years ago

Hello all, just checking in on this review, is there anything I can help you with?

matbesancon commented 4 years ago

still a few code-related issues we are discussing with the author on my side

juliohm commented 4 years ago

Thank you for checking the status of the submission Melissa. The author didn't address all the issues raised. I can take a look again if any update is triggered.

On Fri, Jan 10, 2020, 09:29 Mathieu Besançon notifications@github.com wrote:

still a few code-related issues we are discussing with the author on my side

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1912?email_source=notifications&email_token=AAZQW3IDEH56V46DBSZXZRTQ5BS3FA5CNFSM4JQQHSFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEITYXBQ#issuecomment-573016966, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3OKQ75NLTGVW75IG3LQ5BS3FANCNFSM4JQQHSFA .

kirui93 commented 4 years ago

Hello @juliohm, I addressed all the issues that you had raised concerning the software paper. All the other issues also that were raised concerning the package are also addressed. You had mentioned some times back that I should comment on all the issues raised on how I addressed them and I did. Please have a look on my comments on the way I addressed the issues.

kirui93 commented 4 years ago

Hello @matbesancon, I gave you a reply on what we were threading on last time. Please have a look on it.

kirui93 commented 4 years ago

Hi @melissawm, thank you for reaching out again

juliohm commented 4 years ago

Thank you @kirui93 for the update. On my side you are ready to go, let's wait for @matbesancon 's comments, he may have further comments to improve the work.

kirui93 commented 4 years ago

@juliohm Thank you so much for your insightful comments as well as your time to review our package and the paper.

kirui93 commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

matbesancon commented 4 years ago

As an update, only https://github.com/kirui93/ScenTrees.jl/issues/15 remains on the documentation side, I'll do a last pass on the paper but it starts to all look good to me, @juliohm do you have something more?

juliohm commented 4 years ago

Everything is good to go on my side @matbesancon , thanks for pinging.

kirui93 commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

matbesancon commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1287/mnsc.47.2.295.9834 is OK
- 10.1007/s10479-015-1994-2 is OK
- 10.1007/s101070000202 is OK
- 10.1137/080718401 is OK
- 10.1137/110825054 is OK
- 10.1007/s10589-015-9758-0 is OK
- 10.1137/15M1043376 is OK
- 10.1137/141000671 is OK
- 10.1016/j.ejor.2016.11.028 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/0167-7152(92)90006-Q is INVALID because of 'https://doi.org/' prefix
matbesancon commented 4 years ago

@kirui93 can you check the DOI above, there is one that needs removing the https://doi.org prefix

kirui93 commented 4 years ago

Yeah. It was a slight mistake in the .bib file. I have removed it.

matbesancon commented 4 years ago

@whedon check references