search

openjournals / joss-reviews

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

[REVIEW]: Shop-Solver: A framework for solving and benchmarking shop scheduling problems #1752

Closed whedon closed 3 years ago

whedon commented 5 years ago

Submitting author: @KingZee (Abdelhakim Zakkari) Repository: https://github.com/KingZee/shop-solver Version: v1.2.0 Editor: @gkthiruvathukal Reviewer: @nnadeau, @nthomasCUBE Archive: Pending

Status

status

Status badge code:

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

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

@nnadeau & @nthomasCUBE, 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 @gkthiruvathukal know.

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

Review checklist for @nnadeau

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @nthomasCUBE

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @nnadeau, @nthomasCUBE 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 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

engnadeau commented 5 years ago

@gkthiruvathukal @KingZee my initial impressions:

Authorship

The second and third authors have made no contributions to the repository:

Significance

Issues

Initial Impressions

gkthiruvathukal commented 5 years ago

@nnadeau Thanks for giving the excellent, constructive feedback. We are going to need more than a GUI wrapper to proceed with this submission. When I decided to put this forward for review, I was under the impression there was more of an API here and not just a GUI wrapper.

@KingZee Please take this feedback into account and let us know when you have been able to address the above. You should probably view the above feedback as a need for major revisions. You might find it convenient to make issues in the issue tracker for each of the key bullet points, including the issue about the list of authors. Once all of these issues are resolved, we can continue review. Alternately, we can make a checklist from @nnadeau's report.

KingZee commented 5 years ago

@gkthiruvathukal @nnadeau, thanks a lot for your efforts.

I have to say I am a bit confused, but really willing to discuss and improve on anything that needs work here. What do you mean by a "GUI wrapper"? A wrapper to what? My own code for solving schedules?

What I built here is a documented structure that allows any future development to be extended from the same abstract class. You should know every piece of code in my project is mine, and the GUI isnt a wrapper for a single solver, it's an extension I myself chose to add that is agnostic to whatever solver type is used, making the software extremely easy to fork and improve upon. The Solver  (and Benchmark) classes are made to handle solver work across multiple threads.

The solvers package has 2 solver algorithms that I coded and optimised myself for all 3 types. Like I mentioned before and in the readme, I plan to add many more solver types based on the same class structure explained above and in the Readme bullet points.

So, I really dont understand @nnadeau's argument here, the software doesn't just render a GUI, it actually solves any given matrix. The GUI is even more extra effort I added to make this software even more useful from an academic standpoint.

KingZee commented 5 years ago

I even explained in my paper every step on how I modeled and made the conception of the exact algorithms in my software, which is why I am really confused on what am I missing here to make this clearer.

engnadeau commented 5 years ago

@KingZee please see my comments below:

KingZee commented 5 years ago

Thanks for the comments.

The solution space is for the exact, not the quick solver algorithm. I didn't mention anything about the quicksolver in the algorithm because it is a heuristic I am still trying to improve and maybe write a future paper on. This includes your second comment, I believe that part is not something to be included in the JOSS scope, in here I am including only my software, and the quick solver is included as a personal proof of concept.

For documentation I included explanation and input/output of every class/method, but I can absolutely provide better docs, I will look into the 2 examples you provided.

I did not use an interface, I used an abstract class. Interfaces aren't just a Java concept, it's an OOP structure choice. I could have chose to made the main Solver class either an interface, or an abstract class. Both of them come with different usecases and choices, I chose an abtstract class because some of the Solver class method need bodies because some of the threading code is common across all solvers.

As for your two last comments I am not sure in which category of the JOSS publication requirements you would include that. There are solvers like mine exclusively for TSP, for minimum spanning tree problem, and so on. I feel like you are being slightly subjective and too harsh with judging code quality. If it's because the number of algorithms is lacking that is something that I will be spending months and years researching to keep expanding and improving on this software. What I would like is some exact and specific things I can improve on if you feel like it is lacking, but there are absolutely respectable software created for the point of solving well known combinatorial problems. Not only is there no software like mine written in java, but there is no software that solves shop scheduling problems at all. The fact that this is open source helps even more in that other reaearchers can extend from the Solver abstract class and make their own implementation of solveMakespan(). Thank you.

KingZee commented 5 years ago

I would love to hear the thoughts of @gkthiruvathukal and @nthomasCUBE as well about all of this, to be able to reach a consensus from diverse points of view.

nthomasCUBE commented 5 years ago

!!! REVIEW NOT COMPLETE YETkdntribution of the other author in paper or github.

SOFTWARE aspect

-> authors

i agree that role of coauthors should be stated somewhere. Maybe they can be added into the github project.

-> installation Tool can be run easily with the description described on github. However, it does not work under the latest Java version. Maybe on can describe how the tool can be build and started in Eclipse where '--module-path C:/Users/thoma/Desktop/javafx-sdk-11.0.2/lib --add-modules javafx.controls,javafx.fxml' has to be put in 'Run Configuration'->'Arguments'->'VM arguments' and maybe on can link directly to the jdk-11.0.4 download site.

-> software

gui looks nice and easy to use, probably would be nice to have a possibility to upload a matrix instead of manually adding values.

-> comparison to other tools missing

would be nice to list which other tools do similar analysis and whether the calculation time is better/worse etc.

PAPER aspect (not done yet)

p1. NP-hard, maybe to add a ref that these problems are NP-hard p2. would like to see on the github page an explaination how own algorithms can be included into the tool

Hi guys,

so far i still struggle with compiling and running, managed to use jre11 but now have to manage to make it run,.. then I will provide the comments probably in the next week or so hopefully

gkthiruvathukal commented 5 years ago

Hi, I plan to comment after both reviewers chime in.

Just a reminder that the purpose of review is to identify issues leading to improvements by the authors. So please focus on concrete issues and use the issue tracker. Many reviewers open up specific issues using the issue tracker. Once these are addressed (possibly with back and forth with author) we can move forward.

The goal is to ensure quality of the accepted submission. Hope that helps.

KingZee commented 5 years ago

Hi @nthomasCUBE, thanks a lot for the comments. I have made a new joss branch, so as to get everything working there before merging with master.

SOFTWARE aspect

-> authors

i agree that role of coauthors should be stated somewhere. Maybe they can be added into the github project.

It's true that they are not contributors to the development of the project, but both of them have proof-read and partially co-authored the paper. I could ask them to make github accounts and make symbolic contributions if absolutely necessary. Maybe @gkthiruvathukal can chime in.

-> installation Tool can be run easily with the description described on github. However, it does not work under the latest Java version. Maybe on can describe how the tool can be build and started in Eclipse where '--module-path C:/Users/thoma/Desktop/javafx-sdk-11.0.2/lib --add-modules javafx.controls,javafx.fxml' has to be put in 'Run Configuration'->'Arguments'->'VM arguments' and maybe on can link directly to the jdk-11.0.4 download site.

Thank you, I have made n edit to the readme added a link to JDK 11. (all of the changes are going to be in the JOSS branch for now). Also since each compiler has their own VM arguments prompt, I feel that including explanation on the exact steps for how to add the VM arguments isn't too relevant, as the VM arguments themselves should be enough.

-> software

gui looks nice and easy to use, probably would be nice to have a possibility to upload a matrix instead of manually adding values.

This is a very good feature request. I will try to implement it as soon as I have the time.

-> comparison to other tools missing

would be nice to list which other tools do similar analysis and whether the calculation time is better/worse etc.

As I mentioned before, I have found it very difficult to find software with a goal of scheduling jobs, when I looked for software that solves all job-shop, open-shop, and flow-shop problems I couldn't find any at all. There are some software that simply work as "GUI wrapper"s, and do not provide any solving framework. I have still found some software that generate Gantt charts for the job-shop variant. The motive of this software was in fact the lack of such a framework. There are algorithm implementations and libraries such as (https://github.com/fengelhardt/LiSA), with a large number of algorithms written across years of research, but there isn't that framework to help speed up setup time, as from the literature that I've read it simply seems each researcher re-writes their own implementation of shop scheduling from scratch, and only then proceed to code the solving part.

I reckon I still have to do my due diligence and make an exhaustive in-depth search of all software used here, and give a final answer backed with references.

PAPER aspect (not done yet)

p1. NP-hard, maybe to add a ref that these problems are NP-hard p2. would like to see on the github page an explaination how own algorithms can be included into the tool

I will add a section for contribution and explain extension of the Solver class.

I apologize if this reply is a bit late, comment edits don't trigger a notification so I just noticed your progress as I checked on this. It would be convenient if you raised an issue or made a new comment here for future review comments you might have. Thank you for your effort.

gkthiruvathukal commented 5 years ago

All, ok, now that there is reviewer feedback (thanks @nnadeau and @nthomasCUBE) and a response from @KingZee, we are heading in the right direction. Please keep in mind that JOSS operates differently from your typical academic journal. As an editor, I rarely put anything up for review that lacks some novelty or technical merit, and I see potential in this work. Ultimately, the formal peer review process is designed to improve the quality of the software submitted. I'm therefore going to focus on the quality issues. Being completely novel is not a prerequisite for JOSS submissions but clear motivation for why this is research software and having an audience (statement of need) is. Therefore, we will need to keep these in mind as we move forward.

I'm going to attempt to synthesize but here is a checklist of things I think need to be done before I can recommend acceptance, which is a union of the reviews + my own additions.

I don't think any of the above is impossible to address. While it may take some time, we can work with your schedule, @KingZee. As long as you are willing to see the submission through the process, we can eventually get to a proper accepting state.

KingZee commented 5 years ago

@gkthiruvathukal Thank you very much for your bullet points, I absolutely agree with them and feel that they all are very pertinent. I will do my best over the course of the next few weeks to check off everything.

I have a single concern about the second half of the third bullet point. I agree with both of you and @nnadeau that javadoc might not have been the best choice, and will do my best to re-write all of it in a repo-friendly manner.

But :

The points about interfaces (an absolute must in professional Java programming), using proper logging frameworks, and Thread (concurrency) management need to be addressed.

These are points that might need to be clarified.

  1. There is a total of 4 System.out.println lines in all of my classes. Three of them are simple flags that print "Event running in background", "Event failed", and "Event canceled", which is already superfluous as I am already handling every class of throwable errors in the failed event for example. I could simply delete the lines to absolutely no effect on the code, which really just makes me feel they were low-hanging nitpicks to judge the quality of my code. I will proceed to delete them, I just didn't expect them to be that much of a topic here. I apologize for thinking use of System.out is "okay", because apparently it is not.

  2. Thread management is a point that might need to be discussed on a separate issue, I invite @nnadeau to open one in the repo. The use of Thread.sleep() was justified in my opinion as the heavy nature of the computation would take up so much resources it hinders any use of the machine otherwise. But again, this is up for discussion.

  3. I still am heavily not convinced about the use of interfaces, especially because the switch to using them would almost be trivial : As I am currently using an abstract Solver base class that handles all the threading code, all I would need would to take out the corresponding methods into an interface, and make this Solver class implement it.

But while an interface defines the contract future developers will need to implement, an abstract class inheritance still creates a much stronger contract, I can demonstrate that through many examples :

Trust me I have had this discussion with myself, other developers, and have convinced myself of the use of abstract classes in this specific scenario. I understand that interfaces are common ways to define such contracts, but please understand that this was a logical choice with future implementations in mind.

That would be all, I would like to thank @gkthiruvathukal again for the great synthesis, I'll get to working on it as soon as possible. I would also like to thank both reviewers for their work in judging my submission.

gkthiruvathukal commented 5 years ago

Thanks for the follow up, @KingZee. Keep in mind that you don't need to apologize for anything. The purpose of peer review in JOSS is to improve your submission. When it comes to the System.out.println() issue, we just want you to use proper logging, e.g. log4j. Even though I am a computer scientist and software engineer, I recognize that not everyone is necessarily well-versed in every software engineering practice. The reviewers and I are here to make sure what you have created suffers from as few issues as possible, especially if you want others to use (and perhaps adapt) your software for their research needs.

So please take your time to make the various improvements. I'll ask our reviewers to take a second look (or you can do so when ready). Please use my checklist to indicate where you are in the process, ok?

danielskatz commented 5 years ago

I'm going to mark this as paused for now - @gkthiruvathukal, you can remove this mark when there is progress.

gkthiruvathukal commented 5 years ago

I agree with you, @danielskatz. There are a significant number of issues that likely require significant time to be resolved.

gkthiruvathukal commented 4 years ago

@danielskatz @openjournals/joss-eics This submission has been paused since end of October. Should we go ahead and close the issue, for now?

KingZee commented 4 years ago

@gkthiruvathukal I apologize for taking a while, but since that time I got caught up with other academic aspects of my phd (and other personal reasons), so I unfortunately had to delay working on this. I am definitely still interested in getting my paper & software published on JOSS, but I didn't think there was a deadline on this. If you guys have organizational reasons for closing the issue, I'd be fine with that, otherwise I will try to make serious time for this within the next month.

gkthiruvathukal commented 4 years ago

@KingZee There's no deadline, but I would like to have some estimate of when you can bring it to conclusion! I totally understand the need to focus on finishing your PhD and wish you the best with that! Can you give me a date to follow up with you? We'll keep this issue open.

KingZee commented 4 years ago

Thanks a lot. I will try to get started on fixing all the issues after the first week of February, I will probably create issues on my own repo and ping you and the reviewers after making the relevant PRs.

gkthiruvathukal commented 4 years ago

@KingZee Just checking in. Let us know when you are ready for us to take another look. Ok?

danielskatz commented 4 years ago

👋 @KingZee - any progress to report?

KingZee commented 4 years ago

@danielskatz I have currently began working on the mentioned issues. https://github.com/openjournals/joss-reviews/issues/1752#issuecomment-536621094 This comment summarized what needs work, I closed an issue related to the 3rd point just a few days ago, https://github.com/KingZee/shop-solver/issues/3 . I'm currently figuring out better documentation, I used doxygen as recommended but I did not see any benefit over javadoc, so I am figuring out better alternatives. I will probably make a new commit on the issue soon.

danielskatz commented 4 years ago

👋 @KingZee - any further progress to report? Can you provide an estimate on when the review will be able to progress again?

arfon commented 4 years ago

Dear authors and reviewers

We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required.

Thanks in advance for your understanding.

Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team.

arfon commented 4 years ago

@KingZee - do you think you will have the time to restart working on this submission within the next few weeks or so? If not, in order to respect the reviewers' time and availability committment, I will propose to close (reject) this submission, and invite a resubmission at a later point in time.

KingZee commented 4 years ago

@arfon Absolutely, I was working through the bullet points but due to COVID I was unfortunately unable to continue work on anything for the last quarter or so, I will now get back into finishing up what was left. Apologies for the delay.

gkthiruvathukal commented 3 years ago

@openjournals/joss-eics This submission has been dormant for some time. @arfon and I have both gently nudged @KingZee but it appears progress has stalled. We can follow @arfon's earlier suggestion and close (reject) this for now and give @KingZee the option to resubmit in the future.

arfon commented 3 years ago

OK thanks @gkthiruvathukal.

@nnadeau, @nthomasCUBE, @gkthiruvathukal - thanks for your efforts on this paper. Unfortunately it looks like this one isn't going to make it through review this time.

@KingZee - we'd welcome a resubmission in the future but at this point will assume you're no longer interested or able to pursue this paper in JOSS and therefore move to reject the submission.

arfon commented 3 years ago

@whedon reject

whedon commented 3 years ago

Paper rejected.