whedon commented 3 years ago

Submitting author: @jbuisine (Jérôme BUISINE) Repository: https://github.com/jbuisine/macop Version: v1.2.0 Editor: @melissawm Reviewer: @stsievert, @torressa Archive: 10.5281/zenodo.4595986

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

Status badge code:

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

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

@stsievert & @torressa, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
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 start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @stsievert

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@jbuisine) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
- Specifically, this claim is made in paper.md: "Solutions modeling continuous problems can also be created by the anyone who wants to model his own problem", but I'm not seeing a continuous optimization problem in the examples or tests.
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @torressa

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@jbuisine) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @stsievert, @torressa it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

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 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/0-306-48056-5_11 is OK
- 10.1109/TEVC.2007.892759 is OK
- 10.1016/j.cor.2006.02.008 is OK
- 10.1109/TEVC.2013.2239648 is OK
- 10.1007/s00158-004-0465-1 is OK
- 10.1109/ICMLA.2007.35 is OK
- 10.3390/rs10071117 is OK

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

melissawm commented 3 years ago

👋🏼 @jbuisine @stsievert @torressa this is the review thread for the paper. All of our communications will happen here from now on.

Both reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#REVIEW_NUMER so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time (@torressa has already done so). We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@melissawm) if you have any questions/concerns.

whedon commented 3 years ago

:wave: @stsievert, please update us on how your review is going.

whedon commented 3 years ago

:wave: @torressa, please update us on how your review is going.

stsievert commented 3 years ago

I'll try to have a review in by this weekend.

stsievert commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

stsievert commented 3 years ago

@whedon commands

whedon commented 3 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

stsievert commented 3 years ago

@melissawm I have a review waiting. It looks like I didn't accept the invitation to collaborate on this repo in time, so I can't edit https://github.com/openjournals/joss-reviews/issues/2812#issue-734575770. Could you resend that invitation?

melissawm commented 3 years ago

Sure, no problem!

melissawm commented 3 years ago

@whedon re-invite @stsievert as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@stsievert please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

stsievert commented 3 years ago

My initial review: I do not believe this software project meets the JOSS review requirements. I welcome questions and comments from @jbuisine so provide clarification or be corrected on points below.

The most significant comments/questions are below. I have also edited https://github.com/openjournals/joss-reviews/issues/2812#issue-734575770 to check various boxes* and add some notes on specific items.

The tests are not sufficient. I only found a single test in setup.py, and not a single doctest as claimed. In this one test, there are many untested classes.
Target audience and statement of need. The main documentation do not identify a target audience or a statement of need. Only one feature of the software, maximum flexibility, is described. Who would want that and why? In paper.md, I see that there's a need for maximum flexibility mentioned in the paper, but why would a user want if they're not doing "thesis work"?

The documentation has some specific problems: it's very general and not detailed, is missing a good API description.

Generality: optimization is very general. When is Macop not appropriate?
- For example, could I use it for my favorite deep learning problem? Could it be used for a continuous convex optimization?
- Or what if the optimization problem I'm using requires sparse arrays or GPUs?
Detailed: the only information core classes have is their name – past that, there's not much detail.
- For example, the documentation for IteratedLocalSearch is basically "[this class is] used to avoid local optima and increave EvE (Exploration vs Exploitation) compromise". What's "EvE", what effect does it have, and how do the various parameters balance it?
- For example, what do C and exp_rate do in UCBPolicy? Does increasing C increase or decrease the importance of exploration? How does changing the value of exp_rate change the exploration/exploitation tradeoff?
API: it's not clear what different policies, mutators and crossovers are used for. What's the data flow of one example? Detailing the input/output of these modules would be useful. I'd expect to see a clear definitions for non-experts.

Some specific questions/suggestions/typos for paper.md:

* "generic and implemented OR algorithms" This is the first definition of "OR". * "Tools for modelling and solving discrete and continuous problems are proposed in the literature." This needs at least one citation. * "Allowing students to quickly develop their own algorithms." This sentence is a sentence fragment: it doesn't express a complete thought. * "finding a point $x \in X$ en that has" What does "en" mean? * "implement the whole available algorithms in the literature" -> "which doesn't implement every algorithm in the literature" * "but let you the possibility to quickly develop and test" -> "but provides the ability to ..." * "The main objective of this package is to be the most flexible as possible and hence, to offer a maximum of implementation possibilities." This sentence doesn't make sense. I think you're trying to convey "The main objective of this package is to provide maximum flexibility, which allows for easy experimentation in implementation." * The second paragraph in motivation needs cleaning up -- I'm having a hard time deciphering "Binary solution was used as appreciation for selected or non-selected feature from the available set of features. The solution was therefore a new model obtained and its fitness which is the score obtained on the test data set. " * $\mathbb{N}$ should be used instead of $\mathbf{N}$

^{* a checked box ([x]) means "satisfied", an unchecked box ([ ]) means "not reviewed" and a crossed box ([ ]) means "not satisfied". A partially satisfied requirement is indicated with "[.]"}

torressa commented 3 years ago

Hi @jbuisine thanks for the submission! Here's my review, I welcome responses and questions in any of the following points. Also, I may have more comments in the future.

Major Issues (general)

Documentation: bad.
- The api is present but badly documented (e.g. "Iterated Local Search Algorithm implementation") and not up to date with the master branch. It needs more elaborate explanations of the implementations and citations where appropriate.
- The README and paper do not present an accurate description of what Macop actually is. For this you have to go through the code. Clearer technical description (in paper too)
- The uses of the toolbox has to be better defined, where is it better to use Macop over exact or other methods.
Testing: bad. Unit tests missing (there is one test in the setup.py file)
LICENSE not completed.
Possibly not a substantial scholarly effort
- I've found the bulk of the algorithmic implementation to be mostly contained in July 2019 and August-October 2020. The rest of the work is re-structuring and packaging. Therefore, raising the question of whether the time spent on this project was greater than 3 months.
- To add to this, as reported by whedon there's 1114 lines of python code. Given the structure of the code (with the nested inheritance and redefinition of functions), it is hard to say whether this ticks the "Substantial scholarly effort" box.
Software paper:
- State of the field: there is no mention of other similar software packages. The author comments on well-known general optimisation packages (which need a proper citation), but nothing related to the methods used in Macop. In a quick google-search I found a more closely related repo: Solid which provides multiple derivate-free algorithms with user-defined objectives. The author probably knows more about this but I suspect there must be others, either way, it should be stated in the paper.
- Quality of writing: the paper needs rereading to correct minor grammatical / orthographical errors. I will provide a list once the other comments have been addressed.

Major Issues (code):

Software usage: the usage of library is not very clear. How does one extract the final solution without looking at the logs? Not the user defined criteria like the score/fitness, but the final solution (i.e. in the knapsack example, the items in the knapsack).
Why is abstract class inheritance only sometimes used? This seems like the right type of inheritance for all the classes.
macop/evaluators: contains a single example and not an implementation
macop/operators: triple nested inheritance makes it hard to determine where functions are actually being implemented (e.g. UCBPolicy inherits from Policy which inherits from Operator). For such simple functionality, e.g definining the method apply, this seems a bit overkill. Additionally, inheritance is not used e.g. macop/operators/policies/UCBPolicy.py could be a standalone class.
macop/solutions/: Same as above, inheritance doesn't do much as functions are overloaded and arguments not passed to the parent. The purpose of these functions is really not clear to me.
In general, the module like structure is really confusing, specially given the small and simple implementations. This makes the imports list to extend a fair bit. I understand that this structure may have been laid out for additional functionality to be introduced. However, if no more work is to be done, to help people understand the code and encourage contributions I suggest a major rework of the code to simplify the structure.

Minor comments

Please get rid of the build files for the documentation. readthedocs can build the docs on commits to master if you set it up.
Relative imports are discouraged by PEP 328 "With the shift to absolute imports, the question arose whether relative imports should be allowed at all".
Remove leading _ for all input parameters, this is common practice for private parameters.
macop/callbacks: they are technically a callback, but it seems more like a custom logger
Is there any way to disable the progress output being printed to screen? I can imagine this needs disabling in normal applications
The knapsack is a nice an simple example, however, I don't think it showcases the best use of this library. As it takes longer to arrive at the optimal solution with 30 elements with Macop than traditional exact methods.
In a standard python package, the init file in the main directory is not needed. init file in macop/ should contain module imports
Contributions file is really long while explanations on how to contribute are short.
Examples docs page is really long (half of it is imports and multi-line comments)
In README:
- Tags in readme should link to ci results and external urls, not images.
- "OR algorithms"? list them.

melissawm commented 3 years ago

Thank you both @stsievert and @torressa for the considerations. @jbuisine do you think this is something you can work on with the reviewers comments? Please let me know in case any of you need clarifications or more information.

jbuisine commented 3 years ago

Thanks to @stsievert and @torressa for your remarks. @melissawm Yes it is something I can work on. I will see to take into consideration each of the remarks and update the Python package accordingly (documentation, examples, structural problems...). I will also take the time to answer each of your questions and queries about the package during the week.

melissawm commented 3 years ago

Hello, all! Any updates, @jbuisine ?

jbuisine commented 3 years ago

Hello @melissawm, I will work on it very soon! Sorry for the delay of the updates

jbuisine commented 3 years ago

Hello,

I wanted to inform you that I have been working on updating the package and all the answers for some time. Everything will be available in the next 2 days. I am really sorry for the delay.

jbuisine commented 3 years ago

Hello @melissawm, @stsievert, @torressa

First of all, I would like to apologise for the delay in responding to your comments. I would like to thank you for your very good remarks, I have modified the document (paper), the package and the documentation accordingly. Please find below my comments in relation to your various questions/comments.

The comments to the different questions are answered for each reviewer in two sections:

the answers arising from the reviewer's comments
the answers to the remarks in the JOSS Journal Specification Grid (or requirement box not checked).

According to a certain recommendation, the answer to reviewers is presented as follows:

[RV] Reviewer Comments
[AA] Authors' Answers

@stsievert

1. Most significant comments/questions

1.1. Tests

[RV] The tests are not sufficient. I only found a single test in setup.py, and not a single doctest as claimed. In this one test, there are many untested classes.

[AA] Updates have been done following this comment using doctest in order to better cover the code.

1.2. Audience

[RV] Target audience and statement of need. The main documentation do not identify a target audience or a statement of need. Only one feature of the software, maximum flexibility, is described. Who would want that and why? In paper.md, I see that there's a need for maximum flexibility mentioned in the paper, but why would a user want if they're not doing "thesis work"?

[AA] The proposed Macop package is now specified as being oriented towards discrete optimization. It therefore targets anyone wishing to respond to a discrete optimization problem initially.

Macop wants to allow users to quickly focus on one of proposed features from its interaction main loop.

1.3. API

[RV] The documentation has some specific problems: it's very general and not detailed, is missing a good API description.

Generality: optimization is very general. When is Macop not appropriate? For example, could I use it for my favorite deep learning problem? Could it be used for a continuous convex optimization? Or what if the optimization problem I'm using requires sparse arrays or GPUs?
Detailed: the only information core classes have is their name – past that, there's not much detail. For example, the documentation for IteratedLocalSearch is basically "[this class is] used to avoid local optima and increave EvE (Exploration vs Exploitation) compromise". What's "EvE", what effect does it have, and how do the various parameters balance it? For example, what do C and exp_rate do in UCBPolicy? Does increasing C increase or decrease the importance of exploration? How does changing the value of exp_rate change the exploration/exploitation tradeoff?
API: it's not clear what different policies, mutators and crossovers are used for. What's the data flow of one example? Detailing the input/output of these modules would be useful. I'd expect to see a clear definitions for non-experts.

[AA] The proposed Macop package is now specified as being oriented towards discrete optimization. However, it remains open to extension for continued optimization. Its main objective is to offer a common interaction loop and to allow users of the package to focus their developments on one of the parts in question. It therefore targets anyone wishing to respond to a discrete optimization problem initially.

All documentation has been updated. It is possible to understand the full features interaction and main behaviour of the package with a complete example. A more complete API is also available.

2. edited #2812 (comment)

Functionality

[RV] Installation: Specifically, this claim is made in paper.md: "Solutions modeling continuous problems can also be created by the anyone who wants to model his own problem", but I'm not seeing a continuous optimization problem in the examples or tests.

[AA] The proposed Macop package is now specified as being oriented towards discrete optimization. However, it remains open to extension for continued optimization but it was not dealt with.

Documentation

[RV] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

[AA] Macop now targets anyone wishing to respond to a discrete optimization problem.

[RV] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

[AA] All documentation has been updated. It is possible to understand the full features interaction and main behaviour of the package. A more complete API is also available.

[RV] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

Yes, but these tests are not sufficient; some modules are untouched. See below for details.

[AA] Updates have been done following this comment using doctest in order to better cover the whole code.

[RV] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Yes, there's some guidelines for contributing code/PRs but it's not super clear.
Yes, there is some guidelines for filing issues/bug reports but it only says "submit an issue with label ...".
There really aren't any guidelines for seeking support past "do not hestitate" when contacting.
I think this could be a lot clearer; I'm leaving this requirement unchecked.

[AA] Some improvements have been added in order to make it clearer how it is possible to contribute to the package.

Software paper

[RV] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided? Not really. It claims to solve every optimization problem; that seems a little to general. I don't know what job/data would motivate using this software.

[AA] The proposed Macop package is now specified as being oriented towards discrete optimization.

[RV] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

[AA] Details have been made in the paper.md document and also in the package documentation.

[RV] State of the field: Do the authors describe how this software compares to other commonly-used packages? Marginally – I think this requirement is partially satisfied. The entirety of the comparison is "Available libraries in the literature did not allow this kind of implementation of an evaluation function quickly"

[AA] Citations have been updated to better situated where Macop.

[RV] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)? See below for specific edits.

[AA] The document has been updated with respect to the various remarks proposed.

@torressa

1. Most significant comments/questions

1.1. Documentation

[RV]

The api is present but badly documented (e.g. "Iterated Local Search Algorithm implementation") and not up to date with the master branch. It needs more elaborate explanations of the implementations and citations where appropriate.
The README and paper do not present an accurate description of what Macop actually is. For this you have to go through the code. Clearer technical description (in paper too)
The uses of the toolbox has to be better defined, where is it better to use Macop over exact or other methods.

[AA] All documentation has been updated. It is possible to understand the full features interaction and main behaviour of the package. A more complete API is also available.

1.2. Testing

[RV] bad. Unit tests missing (there is one test in the setup.py file)

[AA] Updates have been done following this comment using doctest in order to better cover the whole code.

1.3. LICENSE

[RV] not completed.

[AA] Updates have been done in order to well complete the License.

1.4. Scholarly effort

Possibly not a substantial scholarly effort

[RV]

I've found the bulk of the algorithmic implementation to be mostly contained in July 2019 and August-October 2020. The rest of the work is re-structuring and packaging. Therefore, raising the question of whether the time spent on this project was greater than 3 months.
To add to this, as reported by whedon there's 1114 lines of python code. Given the structure of the code (with the nested inheritance and redefinition of functions), it is hard to say whether this ticks the "Substantial scholarly effort" box.

[AA] It is true that the development of the package was done over periods rather than on continuity. Updates to the package have also been offered in the current latest version.

1.5. Software paper

[RV]

State of the field: there is no mention of other similar software packages. The author comments on well-known general optimisation packages (which need a proper citation), but nothing related to the methods used in Macop. In a quick google-search I found a more closely related repo: Solid which provides multiple derivate-free algorithms with user-defined objectives. The author probably knows more about this but I suspect there must be others, either way, it should be stated in the paper.
Quality of writing: the paper needs rereading to correct minor grammatical / orthographical errors. I will provide a list once the other comments have been addressed.

[AA] Updates have been done regarding these comments in order to better situate Macop in litterature.

1.6. Major Issues (code):

[RV]

Software usage: the usage of library is not very clear. How does one extract the final solution without looking at the logs? Not the user defined criteria like the score/fitness, but the final solution (i.e. in the knapsack example, the items in the knapsack).
Why is abstract class inheritance only sometimes used? This seems like the right type of inheritance for all the classes.
macop/evaluators: contains a single example and not an implementation
macop/operators: triple nested inheritance makes it hard to determine where functions are actually being implemented (e.g. UCBPolicy inherits from Policy which inherits from Operator). For such simple functionality, e.g definining the method apply, this seems a bit overkill. Additionally, inheritance is not used e.g. macop/operators/policies/UCBPolicy.py could be a standalone class.
macop/solutions/: Same as above, inheritance doesn't do much as functions are overloaded and arguments not passed to the parent. The purpose of these functions is really not clear to me.
In general, the module like structure is really confusing, specially given the small and simple implementations. This makes the imports list to extend a fair bit. I understand that this structure may have been laid out for additional functionality to be introduced. However, if no more work is to be done, to help people understand the code and encourage contributions I suggest a major rework of the code to simplify the structure.

[AA] Many updates have been done in term of code:

Usage of Macop is now better specified in the documentation of software with a full example.
Inheritance is now always used.
macop.evaluators: is now composed of a class and not just a function. Evaluator is composed of the required data of the problem instance.
macop.operators: Operator is basic abstract class, Mutator and Crossover inherit from Operator. Then implementation of Mutator and Crossover are available. UCBPolicy only inherite from Policy abstract class.
macop.solutions: abstract method is now specified to better understand expected inheritance behaviour.
A major rework have been done to better structure the whole Package using module instead of single class per file.

1.7. Minor comments

[RV] Please get rid of the build files for the documentation. readthedocs can build the docs on commits to master if you set it up.

[AA] I now use the classical gh-pages branch to automatically update the doc using CI.

[RV] Relative imports are discouraged by PEP 328 "With the shift to absolute imports, the question arose whether relative imports should be allowed at all".

[AA] Updates have been done.

[RV] Remove leading _ for all input parameters, this is common practice for private parameters.

[AA] Updates have been done. Instance attributes now possessed the leading _.

[RV] macop/callbacks: they are technically a callback, but it seems more like a custom logger

[AA] Yes it's true, but it enables to reload the algorithm state if interuption.

[RV] Is there any way to disable the progress output being printed to screen? I can imagine this needs disabling in normal applications

[AA] verbose attribute to algorithm is now enable and disable the progress.

[RV] The knapsack is a nice an simple example, however, I don't think it showcases the best use of this library. As it takes longer to arrive at the optimal solution with 30 elements with Macop than traditional exact methods.

[AA] It is true that the knapsack is a very classic example and whose solutions can be quickly found. However, I wanted to keep it as an example as well in the documentation due to its simplicity. I will add examples of other instances of problems later.

[RV] In a standard python package, the init file in the main directory is not needed. init file in macop/ should contain module imports

[AA] Updates have been done into __init__.py main file of macop.

[RV] Contributions file is really long while explanations on how to contribute are short.

[AA] I tried to update it to make it a little more clearer.

[RV] Examples docs page is really long (half of it is imports and multi-line comments)

[AA] I update the whole documentation with sections to describe the package usages.

[RV] In README:

Tags in readme should link to ci results and external urls, not images.
"OR algorithms"? list them.

[AA] Some updates have been done in README.md but not sure it was the expected things.

2. edited #2812 (comment)

General checks

I do not fill this part because all comments are answered above.

jbuisine commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

torressa commented 3 years ago

Hi @jbuisine! Thanks for the detailed response and the efforts on the package. I am much happier now with the work you've done so I'm ticking off some of the requirements. Also, really good work on the documentation and diagrams, it looks really nice.

I'll take some time over the week end to come back with my feedback.

torressa commented 3 years ago

Hi again, Here's my feedback

Code

Thanks taking the comments on board and standardising the inheritance structure. The imports look nicer now. I like that you've included placeholders for future implementation for the continuous case.

Only one more thing:

Is it possible to have a more complex example where you have used macop? I like the knapsack example in the docs, but it would be good to have an advanced usage example. If you are using for something in your research, maybe a minimal working example would be good. I've noticed the noise-detection repo, maybe link that it the Macop README.

Paper

Minor comments

Page 1 line 13 repeated "which doesn't"
Page 1 line 16 repeated full stop ".."
Page 2 line 22. The list of optimisation packages is not extensive, I suggest to remove it and just cite software which deals with algorithms closer to Macop. The Perry + Soni Citations should be changed to cite a specific version and year (I've checked and both have releases with year).
Page 2 paragraph starting in page 27 doesn't seem to belong there. The motivation starting in line 35 seems more appropriate to wrap the software discussion.
Page 2 bullets starting in line 47, remove spaces before the semi-colons.
Page 2 line 48 "programming"
Page 2 section beginning on line 61, please refer back to the modules. E.g say in macop.algorihtms. Also, I would include this information in the README.
Page 3 sections "Available solutions", "Update solutions", and "Callback feature", same as above, refer to the module (macop.solutions, macop.evaluators,...) for completeness.
Page 4 line 105, I get what you mean by "dynamic programming of the Python..." but it's a bit confusing as dynamic programming also means something completely unrelated. Please rephrase this.

jbuisine commented 3 years ago

Hi @toressa,

Thank you very mush for your answer.

Code

Regarding the code, I really appreciate your comments. I can indeed propose a more complex example (or even several).

I can use QAP (quadratic assignment problem) or UBQP (unconstrained binary quadratic programming) instances. Instances are proposed on mocobench, but I may stay for the examples in a single-objective context.

In my work on noise detection, this would remain more complicated to explain, in reality, I use a parent algorithm with the real (but very expensive) evaluation function, and then local searches which use a surrogate model (a model that has learned to approximate the real cost function and with a quick-to-evaluate function). I'm afraid this is too far from the generic aspect of the package. But I specified in the paper the interest of the proposed package for this feature (which I specify later in this comment).

So if it suits you, I can add in the documentation some examples on a QAP and UBQP instance.

Paper

Page 1 line 13: fixex
Page 1 line 16: fixed
Page 2 line 22: list updated
Page 2 paragraph starting in line 27: fixed
Page 2 bullets starting in line 47: fixed
Page 2 line 48: fixed
Page 2 section beginning on line 61: fixed
Page 3 sections "Available solutions", "Update solutions", and "Callback feature": fixed
Page 4 line 105: changed to "flexible"
I also added the description in the paper bringing the fact that despite the fact that a dependency exists between different algorithms, each one can have its own context (update operators, evaluation function, solution update policy...). This means that only the results (solutions found) are exchanged for example during the search process.
README.md have been also updated consequently.

Thank you again for your return and usefull comment.

jbuisine commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

torressa commented 3 years ago

Thanks for the fast corrections.

So if it suits you, I can add in the documentation some examples on a QAP and UBQP instance.

That sounds good, would be a great addition!

torressa commented 3 years ago

@whedon commands

whedon commented 3 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

torressa commented 3 years ago

@whedon check references

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/0-306-48056-5_11 is OK
- 10.1109/TEVC.2007.892759 is OK
- 10.1016/j.cor.2006.02.008 is OK
- 10.1109/TEVC.2013.2239648 is OK
- 10.1007/s00158-004-0465-1 is OK
- 10.1109/ICMLA.2007.35 is OK
- 10.3390/rs10071117 is OK
- 10.1007/s00158-011-0666-3 is OK
- 10.1007/978-3-319-42432-3_37 is OK
- 10.1145/3321707.3321800 is OK

MISSING DOIs

- None

INVALID DOIs

- None

melissawm commented 3 years ago

Hello @stsievert - would you be able to take a second look at the paper now that @jbuisine has responded to comments? Let me know if I can help in any way. Thanks!

jbuisine commented 3 years ago

Hello @stsievert, @torressa,

I made some updates:

I updated the paper by naming my thesis supervisors.
I have added some examples with generated QAP and UBQP instances. They are available in the "Some examples" section of the documentation.
I have updated the CONTRIBUTING document to make it lighter and more understandable.

Do not hesitate to let me know if I need to change anything about the paper.

stsievert commented 3 years ago

Thanks for the edits @jbuisine. The documentation is certainly much more descriptive. My comments this time are much smaller; in general, my comments are mostly suggestions. However, I have one blocking comments:

Community guidelines in the recent update, there's no information on users looking to "seek support" (not to my reading at least).
- Also, CONTRIBUTING not rendered as plain text on GitHub (renaming to CONTRIBUTING.md will fix that).
Statement of need: The audience description of this software is much better – the modularity makes sense. However, the description is still very general.
- Who would specifically use this software?
- What are the main features of Macop, and when are they relevant?
- I think the Dask docs are a good example, though it doesn't explicitly mention the audience.

I am still not finished with the review; I need to look closer at the implementation. I also have comments on items that technically meet the requirements:

Functionality documentation:
- Classes don't document initialization methods. Are the arguments stored as class attributes?
- For example, is it required that macop.evaluators.discrete.mono take a dictionary? Type annotations would help a lot.
Usage: I have tried to use your software. I stepped through the example at https://jbuisine.github.io/macop/_build/html/documentations/index.html. This is borderline documentation. Specific comments:
- My development time is bounded. I'd like a simple self-contained script that I can customize -- I'd rather not dig into the different pages. (this comment might be outdated with the recent addition of "some examples").
- Why is necessary to include the pseudo-implementations? (e.g., https://jbuisine.github.io/macop/_build/html/documentations/solutions.html). Why not link to the docs for macop.solutions.base?

I have some more specific notes/style nits:

I would further collapse the knapsack example on the docs into a single HTML page. I found it annoying to click to read a short HTML summary for the first couple sections. (note: it is possible to link to headings/sub-headings).
Why is solution.{_data, _algo} private? Shouldn't it be public? They're both in the tests and the documentation, which is an indication they should be public.
I think the "documentation" link in the documentation sidebar is a meaningless name (of course it's documentation!). Maybe "Example usage"?
I found it unnecesary to include the class implementations at
- This page has unnecessary code that the user doesn't need to write: https://jbuisine.github.io/macop/_build/html/documentations/operators.html
- I think a good example that only writes code the user needs to see is https://jbuisine.github.io/macop/_build/html/documentations/evaluators.html
It'd be really useful to add type annotations to the docs/examples.
I think _build can be removed from the URL; I think you can specify the build location with Sphinx. I'd recommend changing https://jbuisine.github.io/macop/_build/html/documentations/index.html to https://jbuisine.github.io/macop/html/documentations/index.html or https://jbuisine.github.io/macop/documentations/index.html.
For macop.policies.reinforcement.UCBPolicy, does larger C/exp_rate mean more or less exploration? What are typical values?
It'd be really useful it you could provide easier links between types in the documentation (probably easiest by showing more in the table of contents).
- i.e., the documentation for macop.algorithms.mono.HillClimberBestImprovement says the evaluator method takes an object of type Evaluator. I can't easily navigate to see that page.
https://jbuisine.github.io/macop/_build/html/macop/macop.operators.continuous.mutators.html#module-macop.operators.continuous.mutators is empty.
Docstring for macop.algorithms.mono: why are _score and _data private variables? If they're mentioned in the docs and tested in the docstrings, I think they should be public (without the underscore).
Paper, line 60: typo with macop.algorihtms in header.
I find comments in the examples like # policy import and # initializer function with lambda function to be meaningless; that doesn't provide any new information -- initializer = lambda x=20: ... tells me the same information. (I'm looking at https://jbuisine.github.io/macop/_build/html/macop/macop.algorithms.mono.html). I can't read that example because of the comments; don't docstrings allow newlines to separate the code into "paragraphs"?

jbuisine commented 3 years ago

Hello @stsievert,

Thank you very much for your remarks. I updated the package consequently, here my answers on each suggested points:

Blocking comments

[RV]

Community guidelines in the recent update, there's no information on users looking to "seek support" (not to my reading at least). Also, CONTRIBUTING not rendered as plain text on GitHub (renaming to CONTRIBUTING.md will fix that).

[AA] I have added the support seeking information and renamed the contribution file correctly.

[RV]

Statement of need: The audience description of this software is much better – the modularity makes sense. However, the description is still very general. Who would specifically use this software? What are the main features of Macop, and when are they relevant? I think the Dask docs are a good example, though it doesn't explicitly mention the audience.

[AA]

I add the motivation of the package in description of the documentation. I was indeed inspired by the approach proposed for the Dask software. I also add this part into the paper file.

[RV]

Functionality documentation: Classes don't document initialization methods. Are the arguments stored as class attributes? For example, is it required that macop.evaluators.discrete.mono take a dictionary? Type annotations would help a lot.

[AA]

I have updated the documentation by completing the comments on instantiation methods. In addition, I have added typing the data parameter of the Evaluator class as a dict as advised.

[RV]

Usage: I have tried to use your software. I stepped through the example at https://jbuisine.github.io/macop/_build/html/documentations/index.html. This is borderline documentation. Specific comments: My development time is bounded. I'd like a simple self-contained script that I can customize -- I'd rather not dig into the different pages. (this comment might be outdated with the recent addition of "some examples"). Why is necessary to include the pseudo-implementations? (e.g., https://jbuisine.github.io/macop/_build/html/documentations/solutions.html). Why not link to the docs for macop.solutions.base?

[AA]

Indeed, the proposed tutorial allows a complete tour of the use of the Macop's interaction loop. For that, I made it quite complete. As you specified, now there are many faster examples on adding and processing specific problem instances. They are therefore faster to implement and require less time.

In documentation, API links to module have also been added.

Specific notes/style nits:

[RV]

I would further collapse the knapsack example on the docs into a single HTML page. I found it annoying to click to read a short HTML summary for the first couple sections. (note: it is possible to link to headings/sub-headings).

[AA]

I updated the documentation consequently and keep the whole "tutorial" in the single with complete menu in left sidebar.

[RV]

Why is solution.{_data, _algo} private? Shouldn't it be public? They're both in the tests and the documentation, which is an indication they should be public.

[AA]

I have modified the scope of the variables if needed or added accessors/setters if needed. I think it will indeed be more logical to have the code partitioned in the current way.

[RV]

I think the "documentation" link in the documentation sidebar is a meaningless name (of course it's documentation!). Maybe "Example usage"?

[AA]

Indeed, I updated the title into "Tour of Macop", as we take a complete tour of how the package works.

[RV]

I found it unnecesary to include the class implementations at This page has unnecessary code that the user doesn't need to write: https://jbuisine.github.io/macop/_build/html/documentations/operators.html I think a good example that only writes code the user needs to see is https://jbuisine.github.io/macop/_build/html/documentations/evaluators.html

[AA]

I know this is a bit redundant, but it was in the idea of allowing the user following this tutorial to have access to the code and the evolution of the one with the knapsack problem.

[RV]

It'd be really useful to add type annotations to the docs/examples.

[AA]

I don't really understand what you want to say. Type annotations depending of solutions obtained?

[RV]

I think _build can be removed from the URL; I think you can specify the build location with Sphinx. I'd recommend changing https://jbuisine.github.io/macop/_build/html/documentations/index.html to https://jbuisine.github.io/macop/html/documentations/index.html or https://jbuisine.github.io/macop/documentations/index.html.

[AA]

I confess that I haven't found a way to modify this part either on the side of Github or Sphinx. Sphinx allows in its Makefile to specify the output folder but that seems to be all (hence the _build output).

[RV]

For macop.policies.reinforcement.UCBPolicy, does larger C/exp_rate mean more or less exploration? What are typical values?

[AA]

Comments macop.policies.reinforcement.UCBPolicy have been added in order to explain the interest of these parameters with example values and their impacts.

[RV]

It'd be really useful it you could provide easier links between types in the documentation (probably easiest by showing more in the table of contents). i.e., the documentation for macop.algorithms.mono.HillClimberBestImprovement says the evaluator method takes an object of type Evaluator. I can't easily navigate to see that page.

[AA]

In documentation, API links to module have also been added in order to give a better navigration.

[RV]

https://jbuisine.github.io/macop/_build/html/macop/macop.operators.continuous.mutators.html#module-macop.operators.continuous.mutators is empty.

[AA]

This is intended to make it easier for users already wishing to implement continuous optimization problems.

[RV]

Docstring for macop.algorithms.mono: why are _score and _data private variables? If they're mentioned in the docs and tested in the docstrings, I think they should be public (without the underscore).

[AA]

I have modified the scope of the variables if needed or added accessors/setters if needed. I think it will indeed be more logical to have the code partitioned in the current way.

[RV]

Paper, line 60: typo with macop.algorihtms in header.

[AA]

Typo issue fixed into paper.

[RV]:

I find comments in the examples like # policy import and # initializer function with lambda function to be meaningless; that doesn't provide any new information -- initializer = lambda x=20: ... tells me the same information. (I'm looking at https://jbuisine.github.io/macop/_build/html/macop/macop.algorithms.mono.html). I can't - - read that example because of the comments; don't docstrings allow newlines to separate the code into "paragraphs"?

[AA]

I have updated some of the comments and get a better visibility for doctest examples.

Do not hesitate to let me know if some of my answers are not correct. Thanks.

stsievert commented 3 years ago

My replies are below. The documentation is looking a lot better.

I add the motivation of the package in description of the documentation. I was indeed inspired by the approach proposed for the Dask software. I also add this part into the paper file.

Thank you. It's a lot better now. It'd be nice to have a specific example of a use case (but that's not critical).

the documentation for macop.algorithms.mono.HillClimberBestImprovement says the evaluator method takes an object of type Evaluator. I can't easily navigate to see that page. ...

One example: I mean that I can't easily navigate from macop.algorithms.mono.HillClimberFirstImprovment to macop.evaluators.base, even thought it's a returned by HillClimberFirstImprovment.evaluator.

I think this is possible with ref:`~macop.evaluators.base.Evaluator` in Sphinx.

Why is solution.{_data, _algo} private? Shouldn't it be public? They're both in the tests and the documentation, which is an indication they should be public.

I've seen your solution. This code is a little cleaner:

class Algorithm:
    @property
    def data(self):
        return self._data

That allows this usage:

import pandas as pd
alg = Algorithm(...)
d = alg.data
df = pd.DataFrame(d)

It'd be really useful to add type annotations to the docs/examples. ... I don't really understand what you want to say. Type annotations depending of solutions obtained?

For example, macop.evaluators.base.Evaluator.compute takes an argument of type Solution. Presumably that's an instance of macop.solutions.base.Solution. It'd be helpful to have a type hint on that (and even more useful if it's an HTML link).

New comment: it'd be great if you could link to macop.policies.reinforcement.UCBPolicy instead of italicizing that text.

https://jbuisine.github.io/macop/_build/html/macop/macop.operators.continuous.mutators.html is empty. ... This is intended to make it easier for users already wishing to implement continuous optimization problems.

How does that make it easier for users? To modify Macop, they have to write code/tests and submit a PR, or do a development install.

jbuisine commented 3 years ago

Hi @stsievert ,

Thanks for your quick answer and advices. I have made some updates:

Thank you. It's a lot better now. It'd be nice to have a specific example of a use case (but that's not critical).

Inside README.md file, I include a quick explanation why Macop is used inside an other project. It has also been added inside the paper.md file.

One example: I mean that I can't easily navigate from macop.algorithms.mono.HillClimberFirstImprovment to macop.evaluators.base, even thought it's a returned by HillClimberFirstImprovment.evaluator.

I use :class:`~macop.XXXX.XXXX` inside API documentation. It is true that this makes browsing more pleasant. Thanks for the advice.

I've seen your solution. This code is a little cleaner:

I use @property for specific attributes:

Inside Soution: data function (set as @property and @data.setter).
Inside Soution: fitness function (set as @property and @fitness.setter).
Inside Algorithm: result function (set as @property and @result.setter).

The aim is to make it possible to return and process the data/result that the user wants if he creates a new solution/new algorithm.

I have also updated the "A tour of Macop" documentation regarding these changes.

New comment: it'd be great if you could link to macop.policies.reinforcement.UCBPolicy instead of italicizing that text.

This has been updated.

How does that make it easier for users? To modify Macop, they have to write code/tests and submit a PR, or do a development install.

Yes, the user should be able to propose his modifications if he wants them to be integrated in a new version. The idea was to already fix the way the package would be structured to accommodate such modifications.

jbuisine commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

stsievert commented 3 years ago

Nit: https://jbuisine.github.io/macop/_build/html/macop/macop.policies.classicals.html does not completely describe the API for RandomPolicy: it's missing the setAlgo and apply methods.

It looks like this package has two packs: the discrete algorithms/operators/etc, and the data flow that allows customization. Maybe it'd be better to have that separation on the docs? i.e., have all the base classes and other classes well separated in separate sections/page.

Yes, the user should be able to propose his modifications if he wants them to be integrated in a new version. The idea was to already fix the way the package would be structured to accommodate such modifications.

I'm looking at this package from a user perspective, not a developer or contributor perspective. I think an empty module (e.g., macop.solutions.continuous) only confuses users (like me) when they click on the documentation link. I think contributors will contact you with a beta implementation, and you can guide them (either in CONTRIBUTING.md or in the pull request).

jbuisine commented 3 years ago

Thans for your answers.

Here my relative updates:

Display of inheritance classes and members inside API for each class (using 'inherited-members', 'show-inheritance' keywords for autosummary)
API documentation page is now divided in 4 sections:
- Modules description
- Common and base modules
- Discrete Optimisation
- Continuous Optimisation (with some explanations why and guidelines link)

jbuisine commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jbuisine commented 3 years ago

Hi @stsievert, @torressa,

I wanted to inform you that I have also updated the paper (only a few changes).

jbuisine commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

openjournals / joss-reviews

[REVIEW]: Minimalist And Customizable Optimization Package #2812

Status

Reviewer instructions & questions

Review checklist for @stsievert

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @torressa

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Major Issues (general)

Major Issues (code):

Minor comments

@stsievert

1. Most significant comments/questions

1.1. Tests

1.2. Audience

1.3. API

2. edited #2812 (comment)

Functionality

Documentation

Software paper

@torressa

1. Most significant comments/questions

1.1. Documentation

1.2. Testing

1.3. LICENSE

1.4. Scholarly effort

1.5. Software paper

1.6. Major Issues (code):

1.7. Minor comments

2. edited #2812 (comment)

General checks

Code

Paper

Code

Paper

Blocking comments

Specific notes/style nits: