Skip to content
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

deal with python-control#347: missing exception attribute #84

Merged
merged 1 commit into from
Apr 11, 2020

Conversation

bnavigator
Copy link
Collaborator

Enhance the error handling to deal with python-control/python-control#347

@roryyorke
Copy link
Collaborator

This looks fine, and in keeping with the existing ValueError use.

I think it might be cleaner to have a SlycotError, which requires and info argument; then one can't forget the .info attribute. I'll open a separate issue for that.

@roryyorke
Copy link
Collaborator

Unless you have an objection, I'd rather do the rebasing; compare the logs below. Merging gives duplicate log entries. Not the end of the world, but also fairly easy to fix.

What do you think?

Here's the log from fbe8577 rebased onto 196e4ec:

* fbe8577 (HEAD -> fix-pycontrol-347-rebase) fix python-control#347: missing exception attribute
* 196e4ec (upstream/master, master) whitespace fixes
* 2cf6902 replace np.tile with np.repeat wihtout calling np.shape
* 1cc11a4 move td04ad static test
* ef3f2c8 correct API call line for td04ad
* 57bddb4 split test td04ad R and C method
* fe24bbb fix typos, remove obosolete suite function
* 9d2bfe3 (origin/fix_test_sg03ad_static, bnavigator/master) docstrings for sg03ad tests
* 50c9e88 fix sg03ad test for continuos time and add discrete time test
* d10debb remove unused module, clean pep8 warning
* 817105e sg03ad: arange instead of range
*   139b718 Merge pull request #79 from bnavigator/fix-unused-vars
|\  
| *   793625d (bnavigator/fix-unused-vars) Merge branch 'master' into fix-unused-vars
| |\  
| |/  
|/|   
* |   c4ff5af Merge pull request #93 from bnavigator/reorganize-travis
|\ \  
| * | 4e00095 (bnavigator/reorganize-travis) install all test files
| * | 4ea90b0 reorganize build matrix
| * | db51c18 fix SDK dir selection
| * | 49055a3 reorganize travis configuration, remove runtests.py
|/ /  
* |   24bd1d9 Merge pull request #91 from bnavigator/work-on-build-system
|\ \  
| * | ae20794 bla_vendor openblas on CONDA=0

and this is the log after merging this branch to master:

*   bd29dce (HEAD -> deleteme) Merge remote-tracking branch 'bnavigator/fix-control-347' into deleteme
|\  
| *   47e780c (bnavigator/fix-control-347, fix-control-347-rebased, fix-control-347) Merge branch 'test_td04ad_mod' into fix-control-347
| |\  
| | * 5ee39f8 (origin/test_td04ad_mod, bnavigator/test_td04ad_mod) whitespace fixes
| | * ae4446f replace np.tile with np.repeat wihtout calling np.shape
| | * 6603fac move td04ad static test
| | * 690b5e5 correct API call line for td04ad
| | * 36568cb split test td04ad R and C method
| | * 54296df fix typos, remove obosolete suite function
| * |   8cd5b3f Merge branch 'master' into fix-control-347
| |\ \  
| | |/  
| * |   91f6314 Merge branch 'master' into fix-control-347
| |\ \  
| * | | 87d8bf9 fix python-control#347: missing exception attribute
* | | | 196e4ec (upstream/master, master) whitespace fixes
* | | | 2cf6902 replace np.tile with np.repeat wihtout calling np.shape
* | | | 1cc11a4 move td04ad static test
* | | | ef3f2c8 correct API call line for td04ad
* | | | 57bddb4 split test td04ad R and C method
* | | | fe24bbb fix typos, remove obosolete suite function
| |_|/  
|/| |   
* | | 9d2bfe3 (origin/fix_test_sg03ad_static, bnavigator/master) docstrings for sg03ad tests
* | | 50c9e88 fix sg03ad test for continuos time and add discrete time test
* | | d10debb remove unused module, clean pep8 warning

@bnavigator
Copy link
Collaborator Author

I think it is because the rebase and merging strategy are mixed now. You already rebased test_td04ad_mod. If you start at 9d2bfe3 and merge into master (=fast-forward) in the same order as outlined in #98, the log should be clean.

Of course you can rebase, if you want. Please take note of the merge conflict resolves and additions I made after the merges however.

@roryyorke
Copy link
Collaborator

To avoid redoing work you've already done, I'll follow your advice, and start with 9d2bfe3. This does mean, unfortunately, force-pushing to master.

For this particular PR I don't see any merge conflicts - did they come up elsewhere? (I think I had to fix one for test_td04ad_mod, but it wasn't too complex.)

The main thing is not working at cross-purposes or redoing work - it looks like you're quick to merge from master to your PRs, so I basically needn't bother rebasing etc. --- does that sound right?

@roryyorke
Copy link
Collaborator

I've done both options, merging the PRs starting from 9d2bfe3 (result https://github.com/roryyorke/Slycot/tree/rory/merge-all-2), and compared it with a rebase approach (result https://github.com/roryyorke/Slycot/tree/rory/rebase-merge). The only difference is the ordering of files in slycot/tests/CMakeLists.txt, which should be harmless (see below). I'm going to go with the rebase, since my inner fusspot apparently can't let go of the tidier history.

$ git diff rory/merge-all-2..rory/rebase-merge
diff --git a/slycot/tests/CMakeLists.txt b/slycot/tests/CMakeLists.txt
index 6adbe28..1a0c34e 100644
--- a/slycot/tests/CMakeLists.txt
+++ b/slycot/tests/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(PYSOURCE
 
   __init__.py
+  test.py
   test_ab08n.py
   test_ag08bd.py
   test_mb.py
@@ -11,8 +12,9 @@ set(PYSOURCE
   test_tb05ad.py
   test_td04ad.py
   test_tg01ad.py
-  test_tg01fd.py
-  test.py)
+  test_tg01fd.py)
+
+
 
 install(FILES ${PYSOURCE}
         PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE

@bnavigator
Copy link
Collaborator Author

Excellent!

@bnavigator
Copy link
Collaborator Author

The only disadvantage is, that now all the PR pages here on Github show unreadable diffs and by your manual rebase/force-push the PRs won't show as merged.

Could you do the rebase gradually, one PR at a time? Something like

for pr-branch in PRs:
  git rebase --onto master proper-master-startpoint pr-branch
  git push -f bnavigator/pr-branch
  Merge into master on github

@roryyorke
Copy link
Collaborator

I'll do that. I've got local rebased branches for each PR, so it should be straightforward.

@bnavigator bnavigator force-pushed the fix-control-347 branch 2 times, most recently from 79bec1e to fbe8577 Compare April 10, 2020 19:02
@roryyorke roryyorke merged commit b8f3dca into python-control:master Apr 11, 2020
@bnavigator bnavigator deleted the fix-control-347 branch December 31, 2020 13:48
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.

None yet

2 participants