search

JuliaCon / proceedings-review

6 stars 1 forks source link

[REVIEW]: RangeEnclosures.jl: A framework to bound function ranges #122

Closed whedon closed 5 months ago

whedon commented 1 year ago

Submitting author: !--author-handle-->@lucaferranti<!--end-author-handle-- (Luca Ferranti) Repository: https://github.com/JuliaReach/RangeEnclosures.jl Branch with paper.md (empty if default branch): Version: v0.2.4 Editor: Reviewers: @dpsanders, @blegat Archive:

Status

status

Status badge code:

HTML: <a href="https://proceedings.juliacon.org/papers/ec833e982329eeac35354d5d32e4861c"><img src="https://proceedings.juliacon.org/papers/ec833e982329eeac35354d5d32e4861c/status.svg"></a>
Markdown: [![status](https://proceedings.juliacon.org/papers/ec833e982329eeac35354d5d32e4861c/status.svg)](https://proceedings.juliacon.org/papers/ec833e982329eeac35354d5d32e4861c)

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

@dpsanders & @blegat, 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 @odow 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 @dpsanders

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

whedon commented 1 year ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @dpsanders, @blegat 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/JuliaCon/proceedings-review) 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/JuliaCon/proceedings-review:

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 1 year ago

Failed to discover a Statement of need section in paper

whedon commented 1 year ago

Wordcount for paper.tex is 921

whedon commented 1 year ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.11 s (431.5 files/s, 41511.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
TeX                              8            266            178           2393
Julia                           19            128             69            511
Markdown                         6            119              0            323
YAML                             8             12              0            207
Lisp                             1             59              0            142
TOML                             3              5              0             50
Ruby                             1              8              4             45
CSS                              1              0              0              3
-------------------------------------------------------------------------------
SUM:                            47            597            251           3674
-------------------------------------------------------------------------------

Statistical information for the repository '8d948fed977100541e5517c8' was
gathered on 2023/01/23.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
lucaferranti                     1            57              0          100.00

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Luca Ferranti                57          100.0          0.0                7.02
whedon commented 1 year ago

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

whedon commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.29007/zzc7 is OK
- 10.1146/annurev-control-071420-081941 is OK
- 10.1137/141000671 is OK
- 10.1023/A:1021368627321 is OK
- 10.1023/B:NUMA.0000049462.70970.b6 is OK
- 10.1137/1.9780898717716 is OK
- 10.21105/jcon.00097 is OK

MISSING DOIs

- None

INVALID DOIs

- None
odow commented 1 year ago

Pinging @blegat and @dpsanders to check you saw this issue.

lucaferranti commented 1 year ago

Failed to discover a Statement of need section in paper

@odow is it a strict requirement to have a section named this way?

odow commented 1 year ago

I don't think so on the naming front, so long as there is a part in the paper (e.g., the introduction) that still meets the description.

whedon commented 1 year ago

:wave: @dpsanders, please update us on how your review is going (this is an automated reminder).

whedon commented 1 year ago

:wave: @blegat, please update us on how your review is going (this is an automated reminder).

schillic commented 1 year ago

Sending a kind reminder to the reviewers :relaxed:

odow commented 1 year ago

@blegat @dpsanders are you still able to review this? If not, let me know and I can find someone else.

mforets commented 1 year ago

Gentle ping on the status of this submission @odow.

lucaferranti commented 1 year ago

gentle ping @odow

odow commented 9 months ago

I've nudged @blegat to look at this, since he'll be interested in it for some future work. But we should find someone other than @dpsanders

dpsanders commented 9 months ago

Apologies, I should be able to look at this.

blegat commented 9 months ago

Sorry for the delay, I'll do this this week

schillic commented 9 months ago

Thanks all! This is very unfortunate timing :sweat_smile:

The IntervalArithmetic devs decided to change their API and I expect another major breaking change from them in the near future (the changes are already in their main branch). I would prefer to make the paper compatible with that new version before publishing. Concretely, they will remove IntervalBox and instead use AbstractVector{<:Interval}, which we might then do in this package too. I need to discuss this with @mforets and @lucaferranti.

@blegat @dpsanders: You can still take a look at the paper. The only thing I expect to change is the API for multivariate functions. So whenever you see IntervalBox, replace it with AbstractVector{<:Interval}.

schillic commented 9 months ago

About the IntervalBox ... it is complicated ... But, as said above, I think you can go ahead and review. Below is some background.

We have some optional dependencies, which we load in the tests. Four of these packages would need to be updated to the new IntervalArithmetic API before we can test the new version. I have not seen any effort in these packages for the past weeks, and most of these packages are not maintained for more than a year. So it is unlikely that we will get there, and so the user is probably stuck with the current version if these dependencies are used.

Whether we will support newer versions of IntervalArithmetic is currently unclear. I do not like to upgrade IntervalArithmetic and then only run the tests with the old IntervalArithmetic version - who knows whether things still work. But I also do not like to not test functionality with the optional dependencies. Most of the code in this package is actually about that. So there is a conflict. One way out could be to test basic functionality without the optional dependencies and with the latest IntervalArithmetic version, and then switching to another environment for the optional dependencies (or the other way around). But I am not aware of a good way to do that. I guess we would need to have different test folders/environments and different test scripts. But overengineering is rarely a good thing :)

dpsanders commented 9 months ago

I think we should move the current IntervalBox code out into a separate package. If you are able to do this then please feel free.

lucaferranti commented 9 months ago

I think we should move the current IntervalBox code out into a separate package.

@dpsanders maybe open an issue on IntervalArithmetic about this?

blegat commented 9 months ago

The package looks really useful and the paper is clear and well motivated. I only have a few minor comments:

lucaferranti commented 9 months ago

@whedon generate pdf

whedon commented 9 months ago

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

lucaferranti commented 9 months ago

@whedon check references

whedon commented 9 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.29007/zzc7 is OK
- 10.1146/annurev-control-071420-081941 is OK
- 10.1137/141000671 is OK
- 10.1023/A:1021368627321 is OK
- 10.1023/B:NUMA.0000049462.70970.b6 is OK
- 10.1137/1.9780898717716 is OK
- 10.21105/jcon.00097 is OK
- 10.1007/978-0-387-36721-7_6 is OK

MISSING DOIs

- 10.1007/978-3-7091-6918-6_14 may be a valid DOI for title: The wrapping effect, ellipsoid arithmetic, stability and confidence regions

INVALID DOIs

- None
lucaferranti commented 9 months ago

@whedon generate pdf

whedon commented 9 months ago

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

lucaferranti commented 9 months ago

@whedon check references

whedon commented 9 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.29007/zzc7 is OK
- 10.1146/annurev-control-071420-081941 is OK
- 10.1137/141000671 is OK
- 10.1023/A:1021368627321 is OK
- 10.1023/B:NUMA.0000049462.70970.b6 is OK
- 10.1137/1.9780898717716 is OK
- 10.21105/jcon.00097 is OK
- 10.1007/978-3-7091-6918-6_14 is OK
- 10.1007/978-0-387-36721-7_6 is OK

MISSING DOIs

- None

INVALID DOIs

- None
lucaferranti commented 9 months ago

@blegat we have now addressed your review comments. Here is a short summary

You cite [5, 7] saying they provide algorithms but not specifying which and then you mention the branch and bound algorithm without reference. Is the branch and bound algorithm developed in [5, 7] ?

The purpose of the sentence in the introduction was simply to offer a generic motivation and give a more detailed algorithm - reference corresponse in section 2. To avoid confusion, we now added a survey paper on range enclosures methods and cite that in the introduction In section 2, we have a reference for every mentioned algorithm. We have also added a reference for the branch and bound.

"dependency problem and the wrapping effect": do you have any reference for this ?

reference has been added

"where the enclosure obtained with plain interval arithmetic (natural enclosure)": use \empth{natural enclosure} as it's the first time we encounter this term so you are defining it.

done

For [9], you can use the JuliaCon 2019 ref which is more recent

updated

Isn't it weird that the user needs to implement a method that starts with an underscore: _enclose ?

You are right that the _enclose internal method was an extra layer of complexity. The API has now been restructured so that users who want to add new algorithms can simply extend RangeEnclosures.enclose. The paper has beeen updated to reflect this.

"Intervaloptimisation.jl" -> the o should be capitalized

fixed.

Please take a look at the new paper and let us know if you are happy with the new version.

schillic commented 9 months ago

@whedon generate pdf

whedon commented 9 months ago

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

schillic commented 9 months ago

I think we should move the current IntervalBox code out into a separate package. If you are able to do this then please feel free.

@dpsanders: We do not plan to work around the planned IntervalBox removal and rather freeze the IntervalArithmetic version as it is. This is simply because almost all backends have not even started supporting IntervalArithmetic v0.21, so we see little value in trying to support the next breaking version if it would not be usable.

Hence from our side the paper is stable.

blegat commented 8 months ago

The revision addresses all my comments, thanks. Looks good to me now.

lucaferranti commented 8 months ago

@blegat remember to tick the boxes in the message on top ;)

lucaferranti commented 8 months ago

@dpsanders do you have any updates / expected timeline for your review?

lucaferranti commented 8 months ago

@dpsanders any updates?

dpsanders commented 8 months ago

Apologies for the delay in reviewing this.

This is a nice package that brings together in a neat way various different range enclosure methods from a variety of packages in the Julia ecosystem.

Even though this package does not contain that much code, a user could have a difficult time learning to use each of the packages that it wraps, so this is very useful.

The paper is well-written and well-motivated, and the docs are concise and useful.

A couple of comments:

Otherwise I am happy to see this paper published in JOSS!

lucaferranti commented 8 months ago

Thank you for the review @dpsanders , please do make sure to also tick the boxes above :)

You seem to have implemented your own branch-and-bound mechanism in the package. How does this differ from e.g. IntervalOptimization.jl?

The main difference is that in the Moore-Skelboe algorithm one throws away boxes which are proved to not contain an extremum. The implemented branch-and-bound, instead, aggregates boxes into a range extimator as you bisect.

Roughly,

The strategy is very similar to a triditional branch-and-bound, I guess this is technically more a branch-and-accumulate

Maybe you could add an explicit example of how to extend the enclose function with a new method.

This is done in the code snippet at the beginning of section 2.1, any thing in particular you find unclear there that we should clarify?

The EAGO package is a much more sophisticated interval optimization package, which could be useful to wrap as well.

I'll look into it, thanks! I've opened an issue to discuss this

It could be helpful to indicate the computational complexity of each method (although I appreciate that this is non-trivial!)

Indeed, it can be a bit challenging, I am also not sure with respect to what measure the complexity here. With respect to the input dimension? I would expect roughly all of those to suffer from the curse of dimensionality which is typical for inteval methods, but I'll see if we can add some insights about complexity of the algorithms in the paper or documentation.

dpsanders commented 8 months ago

Thanks @lucaferranti, I missed the snippet in section 2.1. If that's not in the actual package docs, then please add it there too.

Thanks for fixing the correctness issue I reported in the repo. Has a new version of the package been released that contains that fix?

schillic commented 7 months ago

@dpsanders:

schillic commented 6 months ago

ping @dpsanders

dpsanders commented 6 months ago

It all looks good to me now, thanks! I've finished my review and completed the checklist, and I'm happy for this to proceed to publication. cc @odow

schillic commented 5 months ago

@odow: Could we push this over the finish line?

odow commented 5 months ago

Oops. Sorry, I was traveling in mid-december and missed this.

odow commented 5 months ago

@whedon generate pdf

whedon commented 5 months ago

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

odow commented 5 months ago

@whedon recommend-accept