search

openjournals / joss-reviews

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

[REVIEW]: MF2: A Collection of Multi-Fidelity Benchmark Functions in Python #2049

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @sjvrijn (Sander van Rijn) Repository: https://github.com/sjvrijn/mf2 Version: v2020.8.0 Editor: @melissawm Reviewers: @torressa, @zbeekman Archive: 10.5281/zenodo.3998591

Status

status

Status badge code:

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

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

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

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

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

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

Review checklist for @petroniocandido

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @zbeekman

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

whedon commented 4 years ago

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

:star: Important :star:

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

To fix this do the following two things:

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

watching

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

notifications

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

@whedon commands

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

@whedon generate pdf
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1007/s00158-014-1213-9 is OK
- 10.1007/s00158-014-1209-5 is OK
- 10.5281/zenodo.2594848 is OK
- 10.1098/rspa.2007.1900 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

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

danielskatz commented 4 years ago

👋 @melissawm - Hi, can you let me know what's going on with this submission. In particular, I only see one reviewer, rather than the required two.

petroniocandido commented 4 years ago

wave @melissawm - Hi, can you let me know what's going on with this submission. In particular, I only see one reviewer, rather than the required two.

Hi @melissawm... Two unforeseen events happened with me and my pregnant wife and took me a long time to solve. I am deeply sorry and I will try to finish the review as soon as possible.

melissawm commented 4 years ago

@danielskatz I'm so sorry, I think I misread the conversation in the pre-review. Can I add new reviewers now?

danielskatz commented 4 years ago

Yes, please do

melissawm commented 4 years ago

Hello @jgoldfar, would you be willing/available to review this software paper for JOSS? Thank you

sjvrijn commented 4 years ago

Hi @melissawm here's the list of potential reviewers again if you're having trouble finding a second one:

And hi @petroniocandido! 👋 Hope everything is fine again with you and your wife! If you've found/noticed anything at all, please let me know! I'm itching to improve my project based on the feedback 😃

melissawm commented 4 years ago

Hello, @torressa! Would you be willing/available to review this software paper for JOSS?

torressa commented 4 years ago

Hi @melissawm! I'd love to! Looks like a really interesting review. I'll reserve some time next weekend.

melissawm commented 4 years ago

@whedon add @toressa as reviewer

whedon commented 4 years ago

OK, @toressa is now a reviewer

melissawm commented 4 years ago

@whedon remove @toressa as reviewer

whedon commented 4 years ago

OK, @toressa is no longer a reviewer

melissawm commented 4 years ago

@whedon add @torressa as reviewer

whedon commented 4 years ago

OK, @torressa is now a reviewer

melissawm commented 4 years ago

Thanks @torressa, let me know if you have any questions!

torressa commented 4 years ago

Hi @sjvrijn! Looks like you've put some work into the package!

I'm not that familiar with evolutionary optimisation algorithms, so please mind the ignorant comments.

I have the following comments:

  1. The pip installation doesn't automatically install the required dependencies. (tested on a clean pyenv).
  2. Also, when trying to run python3 tests/create_regression_data.py I get ImportError: attempted relative import with no known parent package.
  3. You mention in the paper, the numbbo/coco package. Their package is pretty well established, documented, and tested. I've seen that the functions you've implemented are not present, so my question is, why did you decide to release your own package instead of contributing? I think you need to explain, preferably also in the paper, how your package compares to the other(s) and why you choose to release your work separately.
  4. Code-wise:
    • I see there's a fair amount of duplicated code, particularly, the artificialMultifidelity.py and borehole.py (from codacy).
    • Some contributing guidelines would be good.
    • I've got a comment about performance, in your functions, everything is done using vector/matrix manipulations, even though they're pretty efficient in other programming languages, for python, as far as I understand it, it's not usually the fastest way. A quick google search brings up some examples where list comprehensions are better than their numpy equivalent. However, this requires a major overhaul of the package for a (potentially) small increase in performance, so not expecting that. Would be cool though to have some results with increasingly larger instances to see how the implementations scale.
  5. I like the docs, there's explanations, simple examples and how to customise the MultiFidelityFunction class. You mention in the paper that you're using the package for your research, so it'd be good if you could provide a more in-depth example (preferably also with the code). I think the API docs need a bit of work, the docs MultiFidelityFunction are good but the rest of the functions could be improved.
  6. As I said, I'm not familiar with the topic, but are there some benchmark instances to which you can compare your implementations with? Perhaps against the matlab counterparts? You could maybe add this as the in-depth example and kill several birds with one stone.

That's all I've got for now, keep up the good work!

sjvrijn commented 4 years ago

Hi @torressa, thanks for the compliments and comments!

  1. I realised this recently, will add it asap.
  2. I've done most of my development from PyCharm, which tends to run files as-is. Not the first time I've encountered import issues because of it, but hadn't seen this one yet, so thanks for pointing that out 👍
  3. That's a fair question to ask. I'd say it boils down to a difference in the kinds of functions that are used which wouldn't nicely intergrate with coco. I'll update the paper with a more complete comparison and explanation.
    • artificialMultifidelity is semi-dead code at the moment, so you're right, I should properly include or remove it. For borehole.py there are some subtle differences which it doesn't highlight, so not de-duplicating has been a concious decision so far. I'll have a look at it again to see if that's still the right decision.
    • Can you be more specific? Anything specific you would like to see added to the Contributing section of the readme?
    • Matrix/vector operations in numpy are passed through to C/fortran procedures, so as long as the amount of calls is kept to a minimum, they are much faster than pure python loops/comprehensions from what I have experienced. I believe the examples you linked to show the same thing. Meanwhile, the typical evaluation budget is very low (~1000 evals.) in the situations where multi-fidelity optimization becomes relevant. Either way, thanks for suggesting the inclusion of a plot to show how the implementation scales, I agree that can be a useful addition.
  5. I'll see what I can do for a more in-depth but still stand-alone example
  6. Apart from a few implementations here, I don't know of any other public implementations (which is one of the main reasons I wrote this package in the first place 😅 ).

I'll give a notice when I've updated anything.

@melissawm Could you edit the fist post by Whedon to add a check-list for @torressa? Would help me to see what I still have to work on too 🙂

melissawm commented 4 years ago

Hello @sjvrijn ! I'm sorry, I had edited it but for some reason it seems the edit didn't stick, maybe my connection was unstable. Thanks for the heads up!

torressa commented 4 years ago

Hi @sjvrijn! Sorry for the delay and thanks for the reply. I've updated the check point list :sunglasses: Thanks @melissawm

4.2 That looks good actually, missed the subsections on my first look. Also, pretty cool that you set up a gitter channel. 4.3 Fair enough, didn't know that. I just came across several cases when python constructs (maps, generators and list comprehensions) where a lot faster than numpy. I think with the appropriate construct, they are comparable to numpy both when the number of calls is large and when the size of the arrays is also large (which is what happens in all the 2D examples in the link in my original comment, and that's with list comprehensions!). The scalability plot would be great!

  1. Oh cool, I think if the implementations are fairly well known in the field, a simple comparison could help convince people to use your package. It should be fairly easy if you have access to a matlab license.

As for referencing the updates, I'll raise some issues in your repo with the relevant changes. If you add #issue_number to your commit message, github will automatically show it here.

arfon commented 4 years ago

Dear authors and reviewers

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

Thanks in advance for your understanding.

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

sjvrijn commented 4 years ago

First of all, I completely understand it if any of you have more pressing matters than this review given the current circumstances. So please take care of yourself and many thanks for any time you are still able to contribute to this review.

@torressa While creating the scalability plot (coming soon), as a reply to your concerns about performance of using Numpy, I've compared a pre-numpy implementation of the Hartmann6 function with the numpy implementation. As you can see from the table below, the numpy implementation is ~10-80 times faster for evaluating N>=10 points at a time.

N Pure Python Numpy Speedup
1 0.000026 0.000015 1.7
10 0.000229 0.000022 10.5
100 0.002211 0.000045 49.3
1000 0.020856 0.000279 74.7
10000 0.197703 0.005815 34.0
100000 2.094764 0.047761 43.9
1000000 20.371699 0.515337 39.5

For completeness: I've taken the code from commit f22104d3e62a4535f660cad73b8a822cc1f3cef0 and adjusted it. You could test this yourself by adding the code below into the mf2/hartmann.py file

import math

A = _A6[0]
P = _P6[0]
alpha_high = _alpha6_high.T[0]

def pure_python_hartmann6_hf(xx):
    xx = np.atleast_2d(xx)
    results = []
    for row in xx:
        outer_sum = 2.58
        for alpha_i, A_i, P_i in zip(alpha_high, A, P):
            inner_sum = 0
            for xx_j, A_ij, P_ij in zip(row, A_i, P_i):
                inner_sum += A_ij * (xx_j-P_ij)**2

            outer_sum += alpha_i * math.exp(-inner_sum)
        results.append(-(1/1.94) * outer_sum)
    return np.array(results)
sjvrijn commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

torressa commented 4 years ago

Hi @sjvrijn! Cool scalability plot, and pretty neat code to create it as well. About the numpy vs pure python discussion, thanks for making the extra effort. I implemented a faster pure python version with generators and maps but was still slower than numpy. You win!

sjvrijn commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

sjvrijn commented 4 years ago

@whedon set v2020.4.3 as version

whedon commented 4 years ago

I'm sorry @sjvrijn, I'm afraid I can't do that. That's something only editors are allowed to do.

sjvrijn commented 4 years ago

Hi @torressa, I believe I've now addressed all the issues you've raised in https://github.com/sjvrijn/mf2/issues/1 and https://github.com/sjvrijn/mf2/issues/2:

arfon commented 4 years ago

:wave: @petroniocandido - how are you getting along with your review?

torressa commented 4 years ago

@sjvrijn Perfect! Looks really good! All ticked off from me

sjvrijn commented 4 years ago

@melissawm @arfon it's been over 2 months since Petrônio last responded. I realize these are special times, but would it be possible to ask for an additional reviewer?

arfon commented 4 years ago

@sjvrijn - I'd like to wait another couple of weeks before we do this.

melissawm commented 4 years ago

Hello @arfon thanks for responding, I agree. @sjvrijn thanks for your patience

sjvrijn commented 4 years ago

@whedon commands

whedon commented 4 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
sjvrijn commented 4 years ago

@whedon remind @sjvrijn in 3 weeks

whedon commented 4 years ago

I'm sorry @sjvrijn, I'm afraid I can't do that. That's something only editors are allowed to do.

arfon commented 4 years ago

@whedon remind @sjvrijn in 3 weeks

whedon commented 4 years ago

Reminder set for @sjvrijn in 3 weeks

whedon commented 4 years ago

:wave: @sjvrijn, please update us on how things are progressing here.

arfon commented 4 years ago

:wave: @jmadera and @zbeekman - would either of you be willing to review this submission for JOSS? We carry out our checklist-driven reviews here in GitHub issues and follow these guidelines: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html

zbeekman commented 4 years ago

I'd love to! Looks interesting!

arfon commented 4 years ago

@whedon add @zbeekman as reviewer

whedon commented 4 years ago

OK, @zbeekman is now a reviewer

arfon commented 4 years ago

I'd love to! Looks interesting!

Great, thanks so much @zbeekman! I've added a checklist for you at the top of this issue. Please carry out your review in this issue by updating the checklist above and giving feedback in this issue. The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html

Any questions/concerns please let me know.

zbeekman commented 4 years ago

@arfon have the reviewer guidelines changed meaningfully in the past ~3-6 months? I'll skim them again to make sure I don't miss anything but I've reviewed a few JOSS papers before.