[REVIEW]: IWOPY: Fraunhofer IWES optimization tools in Python

editorialbot commented 8 months ago

Submitting author: !--author-handle-->@SchmJo@fraukewiese<!--end-editor-- Reviewers: @BenWinchester, @StewMH Archive: Pending

Status

Status badge code:

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

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

@BenWinchester & @StewMH, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @fraukewiese know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @BenWinchester

📝 Checklist for @StewMH

editorialbot commented 8 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

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

@editorialbot generate pdf

editorialbot commented 8 months ago

Software report:

github.com/AlDanial/cloc v 1.88  T=0.09 s (944.6 files/s, 147697.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          55           2017           3360           5125
Jupyter Notebook                 4              0            884            497
Markdown                        10            149              0            324
YAML                             2             16             18            149
SVG                              2             18              0            136
reStructuredText                 5             44             73             75
DOS Batch                        1              8              1             26
TeX                              1              1              0             21
make                             1              4              6             10
HTML                             1              1              0              9
TOML                             1              2              1              3
-------------------------------------------------------------------------------
SUM:                            83           2260           4343           6375
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 8 months ago

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

OK DOIs

- 10.21105/joss.02338 is OK

MISSING DOIs

- 10.1109/access.2020.2990567 may be a valid DOI for title: pymoo: Multi-Objective Optimization in Python

INVALID DOIs

- None

editorialbot commented 8 months ago

Wordcount for paper.md is 492

editorialbot commented 8 months ago

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

BenWinchester commented 8 months ago

Review checklist for @BenWinchester

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 https://github.com/FraunhoferIWES/iwopy?
[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 (@SchmJo) 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
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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

Note: The documentation can be found here.

[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?
[ ] 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

[ ] 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: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[ ] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[ ] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[ ] 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?

fraukewiese commented 8 months ago

@SchmJo could you check on the missing DOI (see above)? Thank you

StewMH commented 8 months ago

Review checklist for @StewMH

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 https://github.com/FraunhoferIWES/iwopy?
[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 (@SchmJo) 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
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[ ] Functionality: Have the functional claims of the software been confirmed?
[ ] 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

[ ] 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).
[ ] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[ ] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[ ] 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

[ ] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[ ] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[ ] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[ ] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[ ] 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?

BenWinchester commented 7 months ago

@SchmJo, I've carried out as much of the check-list review as possible but have unfortunately encountered a couple of Python ImportError's and raised issues in the iwopy issue tracker (here and here) for these. Unfortunately, until these are resolved, I am unable to verify the functionality, performance, and automated tests.

The remainder of my review is as follows below.

Many thanks, Ben

General comments

I think the repository and paper conform to the JOSS review criteria and recommend it for publication. However, I have a few points, itemised below, which I would like to see addressed.

Substantial scholarly effort

What the repository lacks in citations, it makes up for in its length of commit history and number of lines of code. The repository's nature means that, once published in the JOSS, I believe that its use will continue to expand. I am therefore happy that the repository/paper meet the requirements for substantial scholarly effort, albeit without satisfying each of the criteria.

Checklist comments

Comments as below:

General checks

[x] Contribution and authorship:
- [x] Does the full list of paper authors seem appropriate and complete? I am unclear of the conitrbution that Bernhard Stoevesandt has made to the repository/submission. The repository's contributors page shows contributions from (@SchmJo) only. 05/06/24: @SchmJo confirmed the role that Bernhard Stoevesandt has made and, as authors are responsible for determining co-authorship and it remains unclear, this has been checked off and left for the editor/authors to decide.

Functionality

[x] Functionality:
- [x] Have the functional claims of the software been confirmed? I have encountered an error when attempting to verify by running the instructions here. This has been raised as issue 12 in the target repository. 19/3/24: I have now tested the software and validated that it runs.

Documentation

[x] Automated tests:
- [x] See issue 11 in the target issue tracker. 19/3/24: Having installed the required dependencies, I've tested the automated tests and can confirm that they run.
[ ] Community guidelines:
- [ ] Are there clear guidelines for third parties wishing to ... 3) Seek support? There are no clear ways to seek support through, e.g., an email mailing list, raising an issue, or opening a discussion. This isn't clear either in the README.md file or in the documentation.
[x] :sparkles: 19/3/24: Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
- [x] The installation instructions do not install the dependencies, and, in order to run the automated tests or any of the examples, these need to be installed. Currently, without a good level of documentation, users are expected to run the automated tests, read that an ImportError/ModuleNotFoundError has occurred, and install the dependencies from there. Whilst this is to be expected of code under-development, I believe that, without documentation (whether in the README.md or managed through a package manager), the current iwopy package is not user friendly as it does not contain "a clearly-stated list of dependencies." 05/06/24: Recent changes to the documentation of iwopy make the dependencies required to run the software and examples clear.

Software paper

[ ] Summary:
- [ ] Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided? The summary provides a good introduction to the problem. However, the high-level functionality is not outlined clearly enough
[ ] State of the field:
- [ ] Do the authors describe how this software compares to other commonly-used packages? Currently, there is no State of the field section within the paper.
[ ] Quality of writing:
- [ ] Page 1, line 24, I believe that "the optimizers from the supported liked packages" should read "the optimizers from the supported linked packages." Currenly, the use of "liked" is unclear and informal. If a synonym of "liked" is wanted, potentially "preferred" should be used.
[ ] References: For the references, the editorialbot has highlighted a missing DOI for pymoo. Other than this, I believe that the reference section should be expanded. Including the State of the field section would provide scope for additional references, so I am not greatly concerned as the length of the reference list will naturally increase.

fraukewiese commented 7 months ago

Thank you very much @BenWinchester . @SchmJo : Please update us about your reactions to the comments by @BenWinchester

fraukewiese commented 5 months ago

@SchmJo : Could you please update us regarding your answers to the reviewer's comments? If the review process takes so long it might be more difficult for the reviewers, so it would be great if you could let us know your status.

SchmJo commented 5 months ago

Hi all, thanks for reviewing and thanks for the issues!

I hope that I will be able to address them until mid February. As far as I see it, the issues were due to dependencies on pymoo and pygmo that were needed for the tests, but are optional for the main package. I will improve this, and go through your comments soon.

...and sorry for the delay on my side!

fraukewiese commented 4 months ago

Thanks for the update @SchmJo

fraukewiese commented 3 months ago

@SchmJo : Are there any updates from your side?

SchmJo commented 3 months ago

Yes, I solved the two issues that were posted by the reviewers. Waiting for feedback before closing one of them: #12

BenWinchester commented 3 months ago

Hi @SchmJo and @fraukewiese ,

Following the advice within issue #12 of the iwopy repository, I installed the dependencies and can confirm that the software runs as described. However, whilst this means that I have crossed off the functionality-related requirements, there are still

the other outstanding comments (which are hopefully visible in the updated review and checklist above),
and a new comment (indicated with :sparkles: and the date, 19/3/24) in that, whilst a user can manually install the previously-missing dependencies, there is no clear documentation of these dependencies, either in the user guide, README.md, repository folder etc., which means that I don't believe, in its current form, the repository contains "a clearly-stated list of dependencies." Any of the above methods (README.md, inclusion in the requirements.txt, etc.) would all suffice, so I believe this isn't a major issue, but thought I'd flag so it's kept track of :smiley:

SchmJo commented 3 months ago

@BenWinchester Concerning the deps:

The above discussion is about a dependency that is required for running the tests. It appears here in the setup.cfg file
The testing is described here in the README.md and here in the documentation
Both descriptions explicitly state the instruction pip install -e .[test], which installs these dependencies

Is this insufficient? If you insist, I can make it a general dependency, even though this somewhat breaks the intention of iwopy to not install all packages automatically for which it provides interfaces. Thanks for further feedback

BenWinchester commented 3 months ago

@SchmJo , thanks for getting back to me :smile:

I think there isn't really a "best" way to deal with this dependency. I may be interpreting this wrong (sorry if so!) but it seems as though, iwopy, as a package, works to wrap around other optimisation tools, so it's dependent on them. If it's nicer to have it not install these dependencies (so users can choose their own optimisation packages to install?), then they probably don't need to be automatic. However, there's the check-list comment that needs addressing:

[ ] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

I think, at the minute, listing the dependencies in the setup.cfg as installed only for testing isn't sufficient for a user to run the code. E.G., for someone to run the examples (as I did: they're great fun!), they need to run the code, get the error message, realise that they should install the packages manually, and then all will be well.

This doesn't seem neat enough to me to fulfil the "clearly-stated list" requirement. They could maybe go in the README.md, or, if that breaks the idea that the packages can be selected independently from iwopy, perhaps they could go at the top of each example in the documentation? Something like "to run these examples, you'll need to have pygmo installed. You can run pip install pygmo" or something similar? There's nothing in the checkbox (as far as I can read) that says the process needs to be automatic, just that it needs to be clearly-stated :smiley:

fraukewiese commented 2 months ago

@SchmJo : Are there any updates from your side?

SchmJo commented 2 months ago

I am not sure I understand the issue with the not explicitly mentioned dependencies - it is a setup.cfg-type package. For me it is not nice to explicitly state the dependencies again as pip install ... at several places, since this is redundant and harder to maintain. What is your opinion, @fraukewiese ? Is pip install .[test] really insufficient for the user, before running tests?

BenWinchester commented 2 months ago

@SchmJo , happy to leave this to @fraukewiese . Just to be clear though, my issue is that, to run the code, i.e., to run any of the examples in the documentation (which are great fun!), you need these dependencies installed, and there's currently no guidance for this: a user only finds out when the error message is displayed to them. One of the options I mentioned, which doesn't seem to tricky to implement (as it just requires an update to the source code behind the examples documentation—no changes are needed to the source code of the package), is having a message at the top of the examples with some short instructions:

Something like "to run these examples, you'll need to have pygmo installed. You can run pip install pygmo" or something similar? There's nothing in the checkbox (as far as I can read) that says the process needs to be automatic, just that it needs to be clearly-stated 😃

Please also let me know when you've had a chance to check off the other items in the review and I'll take a look at the repository again :smile:

SchmJo commented 2 months ago

@BenWinchester Ok, thanks! I agree such an addition to the examples is a good way out of this. I will add such a sentence to the notebooks.

fraukewiese commented 1 month ago

@SchmJo : Could you update us on the status regarding the issues raised by @BenWinchester ? Thank you :)

SchmJo commented 1 month ago

@fraukewiese I am on it - my prognosis is end of May (I have vacation time ahead). Hoping that is ok? Sorry for all these delays along the way!

SchmJo commented 1 month ago

@BenWinchester I made the following updates, thanks for checking:

The community is invited here in the documentation to contribute, and here in the README
The setup.cfg file now contains an option for extra dependencies called opt, which will always list all featured third-party optimization packeges (currently pygmo and pymoo). This is explained here in the documentation
The examples section of the documentation explicitly states the dependency on pygmo and pymoo, as requested
Also the testing section of the documentation now mentions the extra dependencies, which are installed by the extra dependency option test in setup.cfg
Explanation: The author "Bernhard Stoevesandt" appears as an author in the role of a "last-author" of the paper. He is my boss and has not written any line of code for this project, but was involved in strategic planing and money aquisition for making it possible. If wanted, I can take him off the author list, not a problem - please just let me know.

I hope this is helpful and can bring this process forward - thanks again for reviewing!

fraukewiese commented 3 weeks ago

@SchmJo Thanks for the updates.

fraukewiese commented 3 weeks ago

@BenWinchester : Are you satisfied regarding the changes made by @SchmJo ? Thank you very much :)

BenWinchester commented 3 weeks ago

@BenWinchester I made the following updates, thanks for checking:

The community is invited here in the documentation to contribute, and here in the README

The setup.cfg file now contains an option for extra dependencies called opt, which will always list all featured third-party optimization packeges (currently pygmo and pymoo). This is explained here in the documentation

The examples section of the documentation explicitly states the dependency on pygmo and pymoo, as requested

Also the testing section of the documentation now mentions the extra dependencies, which are installed by the extra dependency option test in setup.cfg

Explanation: The author "Bernhard Stoevesandt" appears as an author in the role of a "last-author" of the paper. He is my boss and has not written any line of code for this project, but was involved in strategic planing and money aquisition for making it possible. If wanted, I can take him off the author list, not a problem - please just let me know.

I hope this is helpful and can bring this process forward - thanks again for reviewing!

@fraukewiese , apologies for the delay in getting back to you—I'm currently travelling with limited internet access/speed! @SchmJo , thank you for taking the time to go through and address some of the issues raised:

For community contributing, this is good! However, to tick off the requirement of "Are there clear guidelines for third parties wishing to ... 3) Seek support?," I feel there needs to be some way for people to seek support clearly—currently, there's just guidelines on how to fork the repository and open a pull request. Maybe in the same place (or in the README.md file), there could be steps saying that issues can be opened or that users should contact you directly at some email address?
For the dependencies, I feel the additional instructions in the documentation are both good and sufficient to close that aspect!
For Bernhard Stoevesandt, I'm not sure about the authorship criteria... The [JOSS authorship guidelines](https://joss.readthedocs.io/en/latest/submitting.html#:~:text=Purely%20financial%20(such,needed%20after%20publication.) state that "Purely financial (such as being named on an award) and organizational (such as general supervision of a research group) contributions are not considered sufficient for co-authorship of JOSS submissions, but active project direction and other forms of non-code contributions are." If he was simply involved in funding acquisition and general management, this might not be sufficient. However, if he was also involved in discussions along the way as to, e.g., include and exclude from the model, or algorithms to employ, then this is likely sufficient. The next sentence in the guidelines states "The authors themselves assume responsibility for deciding who should be credited with co-authorship, and co-authors must always agree to be listed." As such, if you feel his contributions are sufficient, then this is sufficient (provided that he's aware of the publication submission). I.E., it isn't for me to decide on this one, but I'll tick off the comment and leave the ultimate decision to you or @fraukewiese if there is an issue :smiley:.

The only other outstanding comments from me are all about the content of the paper, not the repo. Has this been updated?

fraukewiese commented 2 weeks ago

@editorialbot generate pdf

fraukewiese commented 2 weeks ago

@BenWinchester : Thanks a lot for your thorough continued review :)

editorialbot commented 2 weeks ago

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

fraukewiese commented 2 weeks ago

@SchmJo : I think @BenWinchester has described the conditions for being mentioned as a co-author very well. So I think it is on your and your potential co-authors side to decide whether Bernhard Stoevesandt has contributed more than purely financial or organisational contributions.

fraukewiese commented 2 weeks ago

@SchmJo : Please notify @BenWinchester and me as soon as you have updated the content of the paper, according to the points raised above by @BenWinchester Thank you :)

openjournals / joss-reviews