mechmotum / cyipopt

Cython interface for the interior point optimzer IPOPT
Eclipse Public License 2.0
227 stars 54 forks source link

FIX: allow for print_level greater than 1 and ENH: allow warm start for dual variables #239

Closed zhengzl18 closed 9 months ago

zhengzl18 commented 10 months ago

fix #234 and #151

moorepants commented 10 months ago

Can you please add unit tests for these changes? Thanks for the contribution.

zhengzl18 commented 10 months ago

Can you please add unit tests for these changes? Thanks for the contribution.

Sorry, I don't quite know how to add unit test. I just tested them with my own codes. Should I add some test script or something?

moorepants commented 10 months ago

We try to add a unit test, for example see https://github.com/mechmotum/cyipopt/blob/master/cyipopt/tests/unit/test_ipopt_funcs.py, to test any new additions/changes.

zhengzl18 commented 9 months ago

That's weird. The checks seemed to fail because of the missing of SciPy. I don't understand what's going on. I've run $ pytest and passed all checks locally.

moorepants commented 9 months ago

Please merge in the tip of master to get the latest fixes for some scipy tests.

zhengzl18 commented 9 months ago

Please merge in the tip of master to get the latest fixes for some scipy tests.

It seems that all the tests I added in test_dual_warm_start.py should be skipped because they were all built upon minimize_ipopt() which involves SciPy. This somehow makes these tests meaningless. Am I right?

moorepants commented 9 months ago

We use the pytest skip decorator to skip the tests if scipy is not installed:

https://github.com/mechmotum/cyipopt/blob/master/cyipopt/tests/unit/test_scipy_optional.py#L23C2-L24C69

zhengzl18 commented 9 months ago

Yeah I know. I mean, since all the tests I added are based on SciPy, skipping the scipy tests would make all these tests meaningless.

moorepants commented 9 months ago

They are not meaningless because we run them all here in the CI system and anyone that has scipy installed and runs the tests will also run them.

zhengzl18 commented 9 months ago

Got it. Thanks for explanation.