Skip to content

Cleanup Rosenbrock tutorial #315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Cleanup Rosenbrock tutorial #315

merged 1 commit into from
Jul 7, 2022

Conversation

DanielVandH
Copy link
Member

This is the first time I've ever made a pull request to a repo, so please let me know if I made a mistake anywhere here.

This edits the Rosenbrock tutorial to fix a few things:

  • Test and Random are not needed.
  • A lot of the defined OptimizationProblems were missing the _p argument.
  • Some variables were defined out of order.

So the reader should now be able to just copy paste all this code without getting errors.

At the end, I changed BBO() to BBO_adaptive_de_rand_1_bin() because it didn't seem like BBO() is an algorithm, it's just an abstract type(?).

The lines

#prob = OptimizationProblem(optf, x0, _p)
 #sol = solve(prob, IPNewton()) # No lcons or rcons, so constraints not satisfied

are commented out because it seems to error with some iteration over Nothing issue. Something to worry about?

I also added some comments that help the reader understand issues with the constraints (as discussed with #291). For example, I didn't understand the first time I read this why I could set cons(x, p) = [x[1]^2 + x[2]^2] and then just have it be not satisfied - maybe my comments help? Also changed some constraints / added a constraint problem to hopefully give more examples here.

This is the first time I've ever made a pull request to a repo, so please let me know if I made a mistake anywhere here.

This edits the Rosenbrock tutorial to fix a few things:

- `Test` and `Random` are not needed.
- A lot of the defined `OptimizationProblem`s were missing the `_p` argument.
- Some variables were defined out of order.

So the reader should now be able to just copy paste all this code without getting errors. 

At the end, I changed `BBO()` to `BBO_adaptive_de_rand_1_bin()` because it didn't seem like `BBO()` is an algorithm, it's just an abstract type(?). 

The lines
```
#prob = OptimizationProblem(optf, x0, _p)
 #sol = solve(prob, IPNewton()) # No lcons or rcons, so constraints not satisfied
```
are commented out because it seems to error with some iteration over `Nothing` issue. Something to worry about?

I also added some comments that help the reader understand issues with the constraints (as discussed with #291). For example, I didn't understand the first time I read this why I could set `cons(x, p) = [x[1]^2 + x[2]^2]` and then just have it be not satisfied - maybe my comments help? Also changed some constraints / added a constraint problem to hopefully give more examples here.
@ChrisRackauckas
Copy link
Member

are commented out because it seems to error with some iteration over Nothing issue. Something to worry about?

Yeah it's something to worry about for the near future. This was last updated before lcons and rcons existed, so it's due for an update.

I set it to run, let's see if it passes.

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #315 (9b6ed39) into master (d07cfe7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #315   +/-   ##
=======================================
  Coverage   74.68%   74.68%           
=======================================
  Files           9        9           
  Lines         241      241           
=======================================
  Hits          180      180           
  Misses         61       61           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ChrisRackauckas ChrisRackauckas merged commit a9be183 into SciML:master Jul 7, 2022
@DanielVandH DanielVandH deleted the patch-1 branch July 7, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants