working TD04AD.f, does not crash any more on edge cases (N=0, NR=0), by repagh · Pull Request #25 · python-control/Slycot

repagh · 2018-06-20T20:50:51Z

While working on supporting MIMO tf -> MIMO ss, I found some fixes had to be done to TD04AD.f

Notably edge cases (empty matrices) caused problems,
Conversion with 'C', did not always work.
Added a few tests in new test_td04ad.py file, checked against Octave result
corrected transform.pyf, to not allocate zero-dim matrices
matching changes in transform.py

- corrected transform.pyf, to not allocate zero-dim matrices - added tests for td04ad, one static, one checked against Octave with a transfer function set.

and works for 'C' as well as 'R' added a few tests

and works for 'C' as well as 'R' added a few tests in new test_td04ad.py file, checked against Octave result corrected transform.pyf, to not allocate zero-dim matrices matching changes in transform.py

repagh · 2018-06-20T21:27:02Z

Ouch! Catch22
Travis fails on the python-control test set, with the old/current python control. The problem in common_den is irrelevant now, since I drastically re-wrote common_den in the matching pull request on python-control. That pull request needs the new Slycot, since td04ad needed to be fixed

use conda-supplied compilers in meta.yaml fix flags for build on osx, specifically conda has python symbols in executable, needs -undefined dynamic_lookup

added / modified for build with FLang on Windows

repagh · 2018-06-27T12:27:06Z

I believe the Trusty-installed openblas is missing dgebal, I did not have the same problem locally with a Xenial installation.

Don't know currently why the CONDA=1 Python 2.7 and 3.5 versions are failing, they seem to be numerically different from the 3.6 version

Back to good old Lapack and Blas then? Can we make it configurable?

murrayrm · 2018-06-27T15:28:18Z

I like the idea of making it configurable, to allow maximum flexibility for people trying to install the code. At the same time, we are going to need conda-forge versions that people can easily download (and so we have to have things working there as well).

The commit history on this is getting pretty complex, with some of the commits above seeming to related to conda/blas/lapack issues rather than the core TD04AD.f fix that I think was the original point of this PR.

Is it worth refactoring our pull requests a bit so that we have one that is associated with Travis CI, one associated with installation more generally (including Windows), on associated with TD04AD.f?

- force installation of slycot from the local channel, avoid conda-force version

repagh · 2018-06-28T14:31:46Z

Wow. It seems we are down to the intermittent "nontrivial imaginary part" error now, which will go away with the new common_den() function in control.

The Python 2.7 and 3.5 CONDA=1 versions were failing because the conda test section (import slycot) would install python 3.6 for the tests. I pinned that now.
Since there is a slycot in conda-forge, and we need to add the conda-forge channel for lapack or openblas, there is a risk that a failing build of slycot results in installation of the conda-forge slycot, which is then being used for the further test. Re-worked the .travis.yml to fix that.
Lapack in conda-forge is apparently packed with blas, no need to add blas as a dependency in the conda recipe.

I agree it has been a messy process, chasing one problem after another. When the td04ad work failed to build properly on travis, I got carried away fixing all kinds of conda and travis issues. Part of the problem was that I had no travis test environment, so it would just be down to committing and pushing. It would be cleaner to refactor, but there would be quite a bit of work involved, and it would be difficult to break things down again to steps that end up at sound endpoints, where (at least part of) the tests are passed. I am pretty confident on the current result, and I fear disentangling the work again might introduce some new errors.

So if you don't object, I would rather have this one and the matching change to python-control merged. That should clear up the Nontrivial Imaginary Part, and lead to some progress on the other PR's too.

murrayrm · 2018-06-28T16:25:01Z

Looking at the changes, it seems like there is a pretty clear split:

Build related changes: .travis.yml, conda-recipe/*, setup.py
td04ad related changes: slycot/*

Can we implement the first set of changes as a separate PR, test them across platforms, and then merge those into master? Then take the second set of changes as a new PR and merge those afterwards?

If the issue is just the extra work required, I can take a cut at generating a cleaner set of PR's based on the changes so far.

murrayrm · 2018-06-28T18:52:05Z

FYI: I checked out repagh/Slycot and tested out the changes on MacOS. I was able to build using conda, but had to explicitly set the python version:

conda config --append channels conda-forge
conda build --python 3.5 conda-recipe

murrayrm

A couple of comments on current snapshot.

murrayrm · 2018-06-28T19:29:22Z

+  # Install fortran compiler, if not using Conda's
  #
-  - sudo apt-get install gfortran
+  #- sudo apt-get install gfortran


Suggest removing this line rather than commenting it out.

murrayrm · 2018-06-28T19:31:01Z

      conda build --python "$TRAVIS_PYTHON_VERSION" conda-recipe;
-      conda install slycot --use-local;
+      conda install -c conda-forge lapack;
+      conda install --override-channels -c local slycot;


Why this way of doing a local install instead of using --use-local flag? I believe --use-local is what is in the standard conda recipe template.

Yes. But --use-local adds the local channel to your set of channels. If the slycot build failed, it would happily install the slycot found in other channels! This happened in the python 2.7 and 3.5 variants.
Since in the overviews of installed modules I saw "local" listed as a channel for the locally built slycot, I assumed that that channel was simply called local. And it works.

Also, --override-channels, which you need to block all others, will not work with --use-local.

murrayrm · 2018-06-28T19:38:56Z

I got an error when trying to compile using python setup.py install. Compilation was OK, but then I tried to load slycot (inside ipython), I got an error:

ImportError: dlopen(/Users/murray/anaconda/envs/test-repagh/lib/python3.5/site-packages/slycot/_wrapper.cpython-35m-darwin.so, 2): Library not loaded: @rpath/libopenblas.dylib
  Referenced from: /Users/murray/anaconda/envs/test-repagh/lib/python3.5/site-packages/slycot/_wrapper.cpython-35m-darwin.so
  Reason: image not found

Is there something that should be getting installed that isn't being installed?

repagh · 2018-06-28T21:33:26Z

openblas from conda-forge?

murrayrm · 2018-06-29T03:43:01Z

This PR was split into PR #25 (TD04AD) and PR #26 (conda).

repagh and others added 7 commits June 5, 2018 00:56

working on getting 'C' version for td04ad checked

dd90059

test adding

72a9875

work in progress

46171e5

- corrected TD04AD.f to handle the case of 'C' and static gain tf

f897e75

- corrected transform.pyf, to not allocate zero-dim matrices - added tests for td04ad, one static, one checked against Octave with a transfer function set.

another fix

3c6f49f

working TD04AD.f, does not crash any more on edge cases (N=0, NR=0),

253ed42

and works for 'C' as well as 'R' added a few tests

working TD04AD.f, does not crash any more on edge cases (N=0, NR=0),

e8edd6f

and works for 'C' as well as 'R' added a few tests in new test_td04ad.py file, checked against Octave result corrected transform.pyf, to not allocate zero-dim matrices matching changes in transform.py

repagh and others added 13 commits June 22, 2018 12:46

fix conda recipe, anaconda 5

54f0510

delete separate build.sh/bld.bat

a2298fd

use conda-supplied compilers in meta.yaml fix flags for build on osx, specifically conda has python symbols in executable, needs -undefined dynamic_lookup

further testing

6d30bd0

- re-worked windows build to use Flang fortran and vc14 linkage

f01ba18

Merge branch 'master' into fix-td04ad

59e920b

added / modified for build with FLang on Windows

Merge branch 'master' into fix-td04ad

9c812db

travis adjustment for openblas use

9a8da81

forgot semicolon in travis.yml

74241a8

fix win/not win distinction in meta.yaml

b4e82cb

adjust linker options for Linux

a6090e3

fix for compile on Darwin/Linux, add library path based on python loc

8a46dd3

ensure new openblas is used

11fe4f5

add conda-forge channel to the conda config

a893db4

repagh mentioned this pull request Jun 27, 2018

Release v0.8.0 python-control/python-control#191

Closed

repagh and others added 6 commits June 27, 2018 22:22

revert to using lapack

d9bef4f

keep the openblas conda recipe too

97a1e94

revert travis to lapack too

2e72514

use lapack libs for non-conda build too

1b8ff5f

correct package names to liblapack-dev and libblas-dev in .travis.yml

2df2163

- do not specify blas for the lapack version meta.yaml

c387595

- force installation of slycot from the local channel, avoid conda-force version

- ensure test is under same python version as build

439558b

murrayrm reviewed Jun 28, 2018

View reviewed changes

repagh mentioned this pull request Jun 28, 2018

Correct td04ad #27

Merged

murrayrm mentioned this pull request Jun 28, 2018

Fixes to the build and test of Slycot, (part of the work in #25) #26

Merged

murrayrm closed this Jun 29, 2018

murrayrm mentioned this pull request Jun 29, 2018

Update slycot to version 0.3.3 #28

Merged

Navigation Menu

Repository navigation

working TD04AD.f, does not crash any more on edge cases (N=0, NR=0), - #25

working TD04AD.f, does not crash any more on edge cases (N=0, NR=0),#25
repagh wants to merge 27 commits into
python-control:masterfrom
repagh:fix-td04ad

repagh commented Jun 20, 2018

Uh oh!

repagh commented Jun 20, 2018

Uh oh!

repagh commented Jun 27, 2018 •

edited

Loading

Uh oh!

murrayrm commented Jun 27, 2018

Uh oh!

repagh commented Jun 28, 2018

Uh oh!

murrayrm commented Jun 28, 2018

Uh oh!

murrayrm commented Jun 28, 2018 •

edited

Loading

Uh oh!

murrayrm left a comment

Uh oh!

murrayrm Jun 28, 2018

Uh oh!

murrayrm Jun 28, 2018

Uh oh!

repagh Jun 28, 2018

Uh oh!

murrayrm commented Jun 28, 2018

Uh oh!

repagh commented Jun 28, 2018

Uh oh!

murrayrm commented Jun 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

repagh commented Jun 20, 2018

Uh oh!

repagh commented Jun 20, 2018

Uh oh!

repagh commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murrayrm commented Jun 27, 2018

Uh oh!

repagh commented Jun 28, 2018

Uh oh!

murrayrm commented Jun 28, 2018

Uh oh!

murrayrm commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

murrayrm Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

murrayrm Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

repagh Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

murrayrm commented Jun 28, 2018

Uh oh!

repagh commented Jun 28, 2018

Uh oh!

murrayrm commented Jun 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

repagh commented Jun 27, 2018 •

edited

Loading

murrayrm commented Jun 28, 2018 •

edited

Loading