check that initial guesses are within bounds. Fixes #355 #358

Peter230655 · 2025-02-13T13:51:40Z

A method, called from solve to ensure that the initial guesses are within their respective bounds. As such a conflict is not fatal for opty, a UserWarning is raised. Should take care of issue #355

opty/direct_collocation.py

opty/tests/test_direct_collocation.py

Peter230655 · 2025-02-13T15:55:05Z

Adjusted as per the suggestions given.

Peter230655 · 2025-02-13T16:32:56Z

If I read it correctly, about 10 examples seem to give problems.
What I could do is this:

set the kwarg to False
over the next so many days, I could look at the offending examples, and set their kwarg to False?

Or what would you suggest?

moorepants · 2025-02-13T16:38:23Z

My suggestion was made above:

We'll have to update any examples or tests to accommodate this. Either by changing initial guess (probably best idea) or setting respect_bounds=False.

moorepants · 2025-02-13T16:39:14Z

Good looking error message!

ValueError: The initial guesses for [c, k] are in conflict with their bounds.

Peter230655 · 2025-02-13T16:46:55Z

Good looking error message!

ValueError: The initial guesses for [c, k] are in conflict with their bounds.

Thanks!
I wasn't sure whether these brackets [....] would be o.k. - but frankly I could not remove them.

Peter230655 · 2025-02-13T16:47:50Z

My suggestion was made above:

We'll have to update any examples or tests to accommodate this. Either by changing initial guess (probably best idea) or setting respect_bounds=False.

I will go over the examples in the coming days, try to fix them and push them if fixed.

opty/direct_collocation.py

moorepants · 2025-02-13T16:54:56Z

opty/direct_collocation.py

@@ -249,9 +254,58 @@ def solve(self, free, lagrange=[], zl=[], zu=[]):
                gives the status of the algorithm as a message

        """
-
+        if respect_bounds == True:


Suggested change

if respect_bounds == True:

if respect_bounds:

opty/direct_collocation.py

opty/tests/test_direct_collocation.py

Peter230655 · 2025-02-15T10:56:16Z

I proceeded as follows:

Where possible, I adjusted the initial guesses to meet the bounds.
where solutions of previous runs were used as initial guesses, I opened the bounds slightly to allow for numerical errors in the solutions.
plot_park2004.py: Here the initial guesses for the gain factors k00.... k13 were very far from their bounds. When I set them to meet the bounds, the results seemed to be a bit worse. So here I set recpect_bounds=False
plot_human_trait.py. Here the initial guess for the earth problem was some previous solution. Its solution is used as the initial guess for the moon problem. The earth solution seemed far outside the bounds. So, I used the initial guess for the earth problem also for the moon problem

Peter230655 · 2025-02-15T13:12:16Z

This test has been running for nearly 2 1/2 hours.
Is this normal, or did something happen?

Peter230655 · 2025-02-15T13:23:58Z

seems to run too long, will start again.

moorepants · 2025-02-16T05:02:53Z

it seems that conda cannot resolve the environment on windows with python 3.10.

yeadon                    1.5.0              pyhd8ed1ab_2    conda-forge
zstandard                 0.23.0          py310he5e10e1_1    conda-forge
zstd                      1.5.6                h0ea2cb4_0    conda-forge
CI detected...
Channels:
 - conda-forge
 - defaults
Platform: win-64
Collecting package metadata (repodata.json): ...working... done
Solving environment: ...working... 
Error: The operation was canceled.

I don't know why that would be, as it has worked before.

moorepants · 2025-02-16T05:05:21Z

Peter, since you are on windows can you make this edit to your conda env file and see if it works locally when creating the environment:

name: opty-dev
channels:
  - conda-forge
dependencies:
  # build
  - setuptools
  # run (includes optional run deps)
  - cyipopt >=1.1.0
  - cython >=0.29.19
  - matplotlib >=3.2.0
  - numpy >=1.19.0
  - python =3.10
  - scipy >=1.5.0
  - sympy >=1.13  # version required for the docs
  # dev
  - coverage
  - ipython
  - pytest
  - pytest-cov
  # docs/examples
  - joblib
  - numpydoc
  - pip <24  # pin required for the gait2d pip install below
  - pydy >=0.5.0  # gait2d
  - pyyaml  # gait2d
  - sphinx
  - sphinx-gallery
  - sphinx-reredirects
  - symmeplot
  - yeadon
  - pip:
    - -e git+https://github.com/csu-hmc/gait2d#egg=gait2d

Peter230655 · 2025-02-16T06:31:31Z

Peter, since you are on windows can you make this edit to your conda env file and see if it works locally when creating the environment:

name: opty-dev
channels:
  - conda-forge
dependencies:
  # build
  - setuptools
  # run (includes optional run deps)
  - cyipopt >=1.1.0
  - cython >=0.29.19
  - matplotlib >=3.2.0
  - numpy >=1.19.0
  - python =3.10
  - scipy >=1.5.0
  - sympy >=1.13  # version required for the docs
  # dev
  - coverage
  - ipython
  - pytest
  - pytest-cov
  # docs/examples
  - joblib
  - numpydoc
  - pip <24  # pin required for the gait2d pip install below
  - pydy >=0.5.0  # gait2d
  - pyyaml  # gait2d
  - sphinx
  - sphinx-gallery
  - sphinx-reredirects
  - symmeplot
  - yeadon
  - pip:
    - -e git+https://github.com/csu-hmc/gait2d#egg=gait2d

So I understand correctly:

the above is a .yml file.
I store it somewhere on my PC
then I create a new environment, called opty-dev, using this .yml file like it is described in opty,
OR would I use this .yml file to update my existing opty-dev environment? This I do not know how to do.

Thanks!

moorepants · 2025-02-16T06:33:45Z

You can do either, but I only changed this line - python =3.10 so it would install python 3.10.

Peter230655 · 2025-02-16T06:45:43Z

You can do either, but I only changed this line - python =3.10 so it would install python 3.10.

So to update I proceed as follows?

I take the yml file above and store it on my PC as, say, update.yml
I open the anaconda promt and put activate opty-dev
then I go to the folder ``cd path/to/folder
then I put conda env update -f update.yml

moorepants · 2025-02-16T06:53:34Z

That will work but only if you first remove the existing opty-dev conda environment.

moorepants · 2025-02-16T06:54:17Z

Sorry, nevermind, if you use "update" then it should be fine. You can also simple add python =3.10 to the yml file you already have in the repo.

Peter230655 · 2025-02-16T06:57:50Z

Sorry, nevermind, if you use "update" then it should be fine. You can also simple add python =3.10 to the yml file you already have in the repo.

This looks easiest for me! Then, when I push, this changed .yml file will also be pushed. Is this o.k.? OIr is this actually what we want to test?

Peter230655 · 2025-02-16T14:35:50Z

Can you create the environment from scratch, not simply update. It is failing to create the environment from scratch in CI.

NO problem! I will use the .yml file in opty, right?

Peter230655 · 2025-02-16T14:56:04Z

I built from scratch as follows:

I deleted the environment opty-dev
I created a new one as explained in the opty documentation. I used the original .yml file there, which is python 3.13.

This took about 90 sec.
Should I do the same thing but change line 12 there from python to python =3.10 ?

moorepants · 2025-02-16T16:32:34Z

Should I do the same thing but change line 12 there from python to python =3.10 ?

Yes, you have to set python = 3.10 to do the same thing in the failing CI build.

Peter230655 · 2025-02-16T16:36:38Z

Should I do the same thing but change line 12 there from python to python =3.10 ?

Yes, you have to set python = 3.10 to do the same thing in the failing CI build.

Clear, will do so. Right now I managed once again to mess up everything :-((
Hopefully I'll fix it tomorrow morning, and report about the timing.

moorepants · 2025-02-17T04:46:58Z

If you merge master the tests should run correctly again. I fixed it in #363

Peter230655 · 2025-02-17T07:07:07Z

Worked !! :-)

moorepants · 2025-02-18T18:58:32Z

opty/direct_collocation.py

+        ----------
+        free : array-like, shape(n*N + q*N + r + s, )
+            Initial guess given to solve.
+


You can add a "Raises" section to describe the ValueError. See the numpydoc documentation.

You can add a "Raises" section to describe the ValueError. See the numpydoc documentation.

I assume you meant this section:

moorepants · 2025-02-18T18:59:21Z

opty/direct_collocation.py

+                        violating_variables.append(local_ts)
+
+            symbole = self.collocator.state_symbols + \
+            self.collocator.unknown_input_trajectories


bit better to do:

a = (longgggggggggggggggggggggg + longgggggggggggggggggggggggggg)

bit better to do:

a = (longgggggggggggggggggggggg + longgggggggggggggggggggggggggg)

So, parenthesis are always preferred over \ ?

according to pep 8 it is

Thanks!
I would have thought the other way around, I will remember in future.

moorepants · 2025-02-18T19:00:32Z

opty/direct_collocation.py

+
+            if len(violating_variables) > 0:
+                msg = f'The initial guesses for {violating_variables} are in '+\
+                f'conflict with their bounds.'


strings don't need \ if you do this:

msg = (f'sstringgggggggggggggggggggggggg' f'anotherstringgggggg')

moorepants · 2025-02-18T19:02:07Z

opty/tests/test_direct_collocation.py

+    par_map = {b1: 1.0,
+               b2: 2.0,
+               b3: 3.0
+               }


preferred formating for dictionaries is:

a = { 'b': 1, 'c': 2, }

You mean 'b1' instead of b1 or you mena the location of } ?

I mean the location of the parentheses and the indentation. Some examples here: https://peps.python.org/pep-0008/#code-lay-out

moorepants · 2025-02-18T19:03:10Z

opty/tests/test_direct_collocation.py

+        uy: (-1.0, 1.0),
+        z: (-1.0, 1.0),
+        uz: (-1.0, 1.0),
+    }


This case seems to have all unknown variables with bounds. Do you tests cases where bounds are not given for some (or all)?

This case seems to have all unknown variables with bounds. Do you tests cases where bounds are not given for some (or all)?

I will check. I will also check, what happens with 'reversed' bounds

moorepants · 2025-02-18T19:03:44Z

opty/tests/test_direct_collocation.py

+    )
+
+    bounds= {
+        a1: (-1.0, 1.0),


What would happen if this was? a1: (1.0, -1.0)? I guess IPOPT may throw an error.

My method would not care, about IPOPT do not know, never tried.
Would this not be a completely nonstandard way to give an interval?

I'm just trying to imagine what inputs may break your function (and thus should be written as tests).

I'm just trying to imagine what inputs may break your function (and thus should be written as tests).

Sorry, I mixed this up my PR for create_linear_initial_guess
With 'reversed' bounds it would not work, but easy to fix. I never thought of this.
(Maybe subconsciously I assumed your guide: garbage in - garbage out... :-) )

opty/direct_collocation.py

moorepants · 2025-02-19T03:21:47Z

opty/tests/test_direct_collocation.py

+    with raises(ValueError):
+        _, _ = prob.solve(initial_guess, respect_bounds=True)
+
+    # D: respect_bounds=False by default. Bounds are ignored.


you could set max iterations to 1 or 2 to save computation time when solve actually runs.

moorepants · 2025-02-19T03:23:29Z

This looks very good. I left a few minor comments, but they aren't necessary to fix for merging.

The only major comment is that you probably need a few more unit tests to check some variation on what type of bounds were set.

Peter230655 · 2025-02-19T05:46:14Z

This looks very good. I left a few minor comments, but they aren't necessary to fix for merging.

The only major comment is that you probably need a few more unit tests to check some variation on what type of bounds were set.

Thanks!

I just checked an example (your drone) with 'reversed' bounds: {F: (100, -100)}
It did not break, but did not get a reasonable solution at all.
So, if I detect a reversed bound, should I raise an error? If YES, what error? ValueError?

moorepants · 2025-02-19T05:50:56Z

I would have thought that IPOPT would raise an error. That makes me wonder why reverse bounds is allowed for IPOPT. Maybe there is a reason you'd want that.

For this PR, we only need to ensure that your new functionality is well tested. Do we want to support reverse bounds with this functionality? If not, we should probably raise an error if reverse bounds are detected. A ValueError sounds correct.

Peter230655 · 2025-02-19T05:58:32Z

I would have thought that IPOPT would raise an error. That makes me wonder why reverse bounds is allowed for IPOPT. Maybe there is a reason you'd want that.

For this PR, we only need to ensure that your new functionality is well tested. Do we want to support reverse bounds with this functionality? If not, we should probably raise an error if reverse bounds are detected. A ValueError sounds correct.

My method will not work for reversed bounds. (I must admit I did not think of this possibility)
So, I will have to check for this possibility.
At least with opty, I have never come across a situation where I would have needed reversed bounds.
So, maybe better for the opty user to get a ValueError, as it likely will not work, even if IPOPT allows it?

moorepants · 2025-02-19T06:13:01Z

So, maybe better for the opty user to get a ValueError, as it likely will not work, even if IPOPT allows it?

I'm fine with opty telling you no for this.

Peter230655 · 2025-02-19T06:51:35Z

So, maybe better for the opty user to get a ValueError, as it likely will not work, even if IPOPT allows it?

I'm fine with opty telling you no for this.

I will do it like this. Maybe here the numpydocs raises could be used to explain why the ValueError is raised?

moorepants · 2025-02-19T06:53:19Z

Yes, that is the purpose of that documentation section.

Peter230655 · 2025-02-19T16:02:28Z

I now check for reversed bounds before I check whether the initial guess meets the bounds. If there is at least one reversed bound a ValueError will be raised.
I test separately for reversed bounds and for violation of the bounds by the initial guess anf for the working of the kwarg in solve.

Can you somehow ignore all these changed simulations? I changed them, when we decided to set the kwarg of solve to True by default, so I tried to adjust the examples accordingly. When we decided to set it to False, I thought I changed them all back, but apparently this did not work.....

moorepants · 2025-02-27T07:58:49Z

What is the status of this PR? Is it ready for a review?

Looks like merge conflicts need to be resolved.

Peter230655 · 2025-02-27T08:08:23Z

What is the status of this PR? Is it ready for a review?

Looks like merge conflicts need to be resolved.

Yes, ready.
Let me try and fix the conflicts and I will push again.

Peter230655 · 2025-02-27T08:17:47Z

Tried to fix the conflict.

moorepants · 2025-02-28T06:01:04Z

There are still outstanding comments from my first review that haven't been addressed.

check that initial guesses are within bounds. Fixes csu-hmc#355

2fcea65

moorepants reviewed Feb 13, 2025

View reviewed changes

opty/direct_collocation.py Outdated Show resolved Hide resolved

moorepants reviewed Feb 13, 2025

View reviewed changes

opty/tests/test_direct_collocation.py Outdated Show resolved Hide resolved

added kwarf to solve. Raise ValueError. Split the test into two tests

7e32433

moorepants reviewed Feb 13, 2025

View reviewed changes

opty/direct_collocation.py Outdated Show resolved Hide resolved

moorepants reviewed Feb 13, 2025

View reviewed changes

opty/direct_collocation.py Outdated Show resolved Hide resolved

changed as per suggestions

f4ebb9d

moorepants reviewed Feb 13, 2025

View reviewed changes

opty/tests/test_direct_collocation.py Outdated Show resolved Hide resolved

Peter230655 added 2 commits February 13, 2025 18:38

combinde two test functions into one

8d8edd5

make initial conditions match their bounds in examples-gallery

2c03e41

Peter230655 closed this Feb 15, 2025

Peter230655 reopened this Feb 15, 2025

Merge branch 'master' into bounds-conflict

0743109

moorepants reviewed Feb 18, 2025

View reviewed changes

moorepants reviewed Feb 19, 2025

View reviewed changes

opty/direct_collocation.py Outdated Show resolved Hide resolved

moorepants reviewed Feb 19, 2025

View reviewed changes

check for reversed bounds

3453588

merged

374ac51

Update opty/direct_collocation.py

0bd33d9

check that initial guesses are within bounds. Fixes #355 #358

Are you sure you want to change the base?

check that initial guesses are within bounds. Fixes #355 #358

Conversation

Peter230655 commented Feb 13, 2025

Peter230655 commented Feb 13, 2025

Peter230655 commented Feb 13, 2025 • edited Loading

moorepants commented Feb 13, 2025

moorepants commented Feb 13, 2025

Peter230655 commented Feb 13, 2025

Peter230655 commented Feb 13, 2025

Choose a reason for hiding this comment

Peter230655 commented Feb 15, 2025 • edited Loading

Peter230655 commented Feb 15, 2025

Peter230655 commented Feb 15, 2025

moorepants commented Feb 16, 2025

moorepants commented Feb 16, 2025

Peter230655 commented Feb 16, 2025

moorepants commented Feb 16, 2025

Peter230655 commented Feb 16, 2025 • edited Loading

moorepants commented Feb 16, 2025

moorepants commented Feb 16, 2025 • edited Loading

Peter230655 commented Feb 16, 2025 • edited Loading

Peter230655 commented Feb 16, 2025

Peter230655 commented Feb 16, 2025 • edited Loading

moorepants commented Feb 16, 2025

Peter230655 commented Feb 16, 2025

moorepants commented Feb 17, 2025

Peter230655 commented Feb 17, 2025

Choose a reason for hiding this comment

Peter230655 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peter230655 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

moorepants Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peter230655 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorepants commented Feb 19, 2025

Peter230655 commented Feb 19, 2025

moorepants commented Feb 19, 2025

Peter230655 commented Feb 19, 2025

moorepants commented Feb 19, 2025

Peter230655 commented Feb 19, 2025

moorepants commented Feb 19, 2025

Peter230655 commented Feb 19, 2025 • edited Loading

moorepants commented Feb 27, 2025

Peter230655 commented Feb 27, 2025

Peter230655 commented Feb 27, 2025

moorepants commented Feb 28, 2025

Peter230655 commented Feb 13, 2025 •

edited

Loading

Peter230655 commented Feb 15, 2025 •

edited

Loading

Peter230655 commented Feb 16, 2025 •

edited

Loading

moorepants commented Feb 16, 2025 •

edited

Loading

Peter230655 commented Feb 16, 2025 •

edited

Loading

Peter230655 commented Feb 16, 2025 •

edited

Loading

Peter230655 Feb 18, 2025 •

edited

Loading

Peter230655 Feb 18, 2025 •

edited

Loading

moorepants Feb 18, 2025 •

edited

Loading

Peter230655 Feb 18, 2025 •

edited

Loading

Peter230655 commented Feb 19, 2025 •

edited

Loading