search

openjournals / joss-reviews

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

[REVIEW]: CEGO: C++11 Evolutionary Globbal Optimization #1147

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @ianhbell (Ian Bell) Repository: https://github.com/usnistgov/CEGO Version: v1.0.0 Editor: @jedbrown Reviewer: @sarats, @sjvrijn, @mmenickelly Archive: 10.5281/zenodo.2649254

Status

status

Status badge code:

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

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

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

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @jedbrown know.

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

Review checklist for @sarats

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @sjvrijn

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @mmenickelly

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. @sarats, it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

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

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

jedbrown commented 5 years ago

@sarats @sjvrijn @mmenickelly :wave: Welcome and thanks for agreeing to review! The comments from @whedon above outline the review process, which takes place in this thread (possibly with issues filed in the CEGO repository). I'll be watching this thread if you have any questions.

mmenickelly commented 5 years ago

Hi, first time reviewing for JOSS. To clarify this process: any comments I have that prevent me from checking off something in the checklist should be raised as an issue in the CEGO repository and not here, right?

And if I have multiple comments, is there a preference for separating them into multiple issues, or combining all comments into one large issue?

jedbrown commented 5 years ago

Small comments, especially anything stylistic or specific to the paper, are fine here. More significant issues, especially those that should have a definite resolution, are better filed as separate issues with the CEGO repository.

sjvrijn commented 5 years ago

A few small comments so far @ianhbell:

mmenickelly commented 5 years ago

Comments: I am not sure from the README.md or the software paper what are the full capabilities of this software. There are some great examples, also provided in Jupyter notebooks, of a MINLP and some highly non convex functions of real-valued variables, but I don't have a clear picture of exactly what CEGO's scope does and doesn't contain.

To fix this, I would like to see:

  1. Better API documentation. Please tell me if I'm not seeing something, but is there no reference description of the individual functions/classes/return types/arguments?
  2. In the software paper, it would help to include a clear description of exactly what types of constraints and what types of objective functions CEGO can handle. Can CEGO exploit derivative information of objectives/constraints when derivatives exist and are available? Most critically, and what I don't know from the examples, can CEGO handle arbitrary analytic or black-box constraints, or can CEGO only handle bound constraints and integer constraints?
  3. I'm unsure if what is provided count as "automated tests". Yes, I was available to get them to work, but do they count as automatic? This is more of a technicality than anything else.
  4. Continuing on point 3, the output in the paper for the shown example (the HundredDigitPlus example) does not match what is obtained from multiple runs, almost certainly due to randomization. Although many runs find the global minimum, some runs fail to find the global minimum within 1000 evaluations. Something needs to be said in the software paper and possibly the documentation about this (expected and totally reasonable) behavior.

Two smaller comments:

  1. Is the amount of Python code shown in the software paper acceptable by JOSS standards?
  2. Echoing a previous comment that the DOI is missing for the 2006 ALPS paper.
ianhbell commented 5 years ago

To everyone, sorry for the delay in dealing with this review...

@sjvrijn I will, as I have done for other projects, bump the revision to 1.0.0 once the review is complete, and at that time I will mint a DOI, and push to PyPI. Thanks for the pointer about the DOI (fixed). Didn't realize that the conference paper had a DOI.

ianhbell commented 5 years ago

Sorry for the delay everyone...

@sjvrijn Thanks for the pointer about the DOI; I fixed that, didn't know that the conference paper had a DOI. I plan to bump the version to 1.0.0 once the review is complete, mint a DOI for the release, and push to PYPI.

@mmenickelly

  1. Good point regarding docs. I have set up to use doxygen to generate docs, and have committed a PDF of the documentation. It's not a big file.
  2. It's strictly unconstrained optimization. Or rather, strictly softly constrained optimization with the ability to enforce integer constraints on values. So no derivatives (which are a major obtain to generate in the general case). I'll update the README.
  3. You're right. I thought I had set up Travis tests for CEGO, but I guess that was another project. I'll do so and get them to fire on every commit.
  4. That's odd. For me, it yielded the global optimum on every run. So I came in and ran it a few times, and indeed, it didn't always get there. So I tweaked the control parameters a bit and added a caveat notice to each notebook along the lines of your comment

A1. I think so, I did something similar for ChebTools. Do you think it is too much or too little? A2. Fixed, see above

ianhbell commented 5 years ago

I have set up the Travis tests (https://travis-ci.org/usnistgov/CEGO), and pushed all changes to the repo. Awaiting round number 2!

labarba commented 5 years ago

Is this ready for the reviewers—@sarats, @sjvrijn, @mmenickelly—to take a second look?

sjvrijn commented 5 years ago

@ianhbell Could you add the installing of the Python interface to the Travis setup? It currently does not fail if this does not work for some reason. Also, which Python versions is it intended/tested for?

ianhbell commented 5 years ago

I have fixed the build issues with binder and now build the Python wrapper in TravisCI as well. All's well at the moment, ready for another look-see.

sjvrijn commented 5 years ago

A few more remarks:

PS: the use of for i in range(len(x)) in the Griewank definition is not wrong, but more 'pythonic' would be:

    for i, xi in enumerate(x):
        sum1 += pow(xi.as_double(), 2)/4000.0
        prod1 *= cos(xi.as_double()/sqrt(i+1))

🙂

ianhbell commented 5 years ago

Thanks @sjvrijn -- those were all great recommendations. I have made all them and am ready for another look-see. Some comments:

0) I wasn't entirely clear what you had in mind here, so I added To get started, you should check out the Jupyter notebooks in the notebooks folder; they demonstrate some of the capabilities of CEGO. 1) The call to pyswarms doesn't satisfy the constraints since pyswarms only does continuous optimization 2) Thanks for the pythonic suggestion. It's a pity that they don't do a better job talking about the enumerate function in the docs of Python. It's only much later on I learned about this very useful function. Indexing x twice is not a huge penalty time-wise, but I agree your version is nicer to read and saves two characters per line

sjvrijn commented 5 years ago

Glad you found them useful!

RE: 0., Based on https://joss.readthedocs.io/en/latest/review_criteria.html#community-guidelines, your community guidelines explicitly covers the first two (contributions and reporting issues) but not yet the third: seeking support. A quick line on what they can do in that case would complete it for me. It could e.g. be raising an issue here on Github, sending you an email, joining a chatroom, etc

Other than that, I have no further comments :)

ianhbell commented 5 years ago

Great, I added 

If you want to discuss or request assistance, please open an issue.
jedbrown commented 5 years ago

@sarats and @mmenickelly, you still have a few unchecked boxes in your reviews. Can you update us on their status and whether there are remaining issues to resolve?

jedbrown commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
jedbrown commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
whedon commented 5 years ago 

OK DOIs

- 10.1145/1143997.1144142 is OK
- 10.1145/1569901.1570011 is OK
- 10.1109/MCSE.2007.53 is OK
- 10.1115/1.2912596 is OK
- 10.1023/A:1008202821328 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 5 years ago

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

ianhbell commented 5 years ago

Looks good to me!

jedbrown commented 5 years ago

Can you please merge the above PR and comment on the 2009 paper that seems to have not been published in the stated venue? Then we can double-check the paper before archiving. Thanks.

ianhbell commented 5 years ago

Hornby paper has been removed in https://github.com/usnistgov/CEGO/commit/d99957e2ffb65d0d632a94c15385e752ac70368a

ianhbell commented 5 years ago

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

jedbrown commented 5 years ago

@ianhbell Looks good. Please tag a release (annotated tags preferred), archive on Zenodo or similar, and report the DOI here.

ianhbell commented 5 years ago

Here it is: 10.5281/zenodo.2649254

Annotated tag added too

jedbrown commented 5 years ago

Thanks. Please update the author info to remove my name.

jedbrown commented 5 years ago

@whedon set 10.5281/zenodo.2649254 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2649254 is the archive.

ianhbell commented 5 years ago

Hmm - where is your name listed as author?

jedbrown commented 5 years ago

In the Zenodo archive; follow DOI link above or direct here: https://zenodo.org/record/2649254

ianhbell commented 5 years ago

Got it -- fixed

jedbrown commented 5 years ago

@whedon accept

whedon commented 5 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 5 years ago

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/640

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/640, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
whedon commented 5 years ago 

OK DOIs

- 10.1145/1143997.1144142 is OK
- 10.1145/1569901.1570011 is OK
- 10.1109/MCSE.2007.53 is OK
- 10.3233/978-1-61499-649-1-87 is OK
- 10.25080/Majora-4af1f417-011 is OK
- 10.1115/1.2912596 is OK
- 10.1023/A:1008202821328 is OK

MISSING DOIs

- None

INVALID DOIs

- None
jedbrown commented 5 years ago

@openjournals/joss-eics We're ready for you.

danielskatz commented 5 years ago

Thanks for the reviews, @sarats, @sjvrijn, @mmenickelly, and for the editing, @jedbrown

danielskatz commented 5 years ago

@whedon accept deposit=true

whedon commented 5 years ago 
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 5 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/642
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01147
  3. If everything looks good, then close this review issue.

  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

    Any issues? notify your editorial technical team...

danielskatz commented 5 years ago

👋 @arfon - can you check on this?

There seems to be a problem with the paper, specifically https://www.theoj.org/joss-papers/joss.01147/10.21105.joss.01147.pdf isn't there.

jedbrown commented 5 years ago

It's there now?