search

CPCLAB-UNIPI / SIPPY

Systems Identification Package for PYthon
GNU Lesser General Public License v3.0
269 stars 92 forks source link

Refactor/armax #7

Closed don4get closed 5 years ago

don4get commented 5 years ago

After having fixed armax examples, I refactored the algorithm so that it is more readable and aligned with PEP8 conventions.

CPCLAB-UNIPI commented 5 years ago

Thanks to everyone for their contribution. We are analyzing the most recent version of this branch considering to merge it into the master one. Most of the problem were caused by the lsim function and they have been solved for most cases. Unfortunately the current version of the branch gives an error in the ARX_MIMO.py, both in Python 2.7 and 3.6, still because the lsim function has a mismatching dimension error. The bottom line is the update between control.__version__ = 0.8.1 and control.__version__ = 0.8.2. A transpose in the timeresp.py at line 354 (in control.__version__ = 0.8.2) gives the error absent for control.__version__ = 0.8.1. Given this, we are planning to substitue the lsim function from the control package with a tailor-made one.

CameronDevine commented 5 years ago

Would it be possible to reshape the output of lsim to get around the transpose issue? I believe this would transpose the output if it is the wrong shape.

CameronDevine commented 5 years ago

As a bit of clarification, reshape performs a transpose if the data is one dimensional (either a row or column vector). However, I still think it would be possible to simply transpose when necessary, either by check the version of the control library, or the shape of the returned data.

CPCLAB-UNIPI commented 5 years ago

Unfortunately it is not the output of the lsim the problem but its input. This would add a control.__version__ dependent if statement in the example file, hence in the user file. To be more general it is better to add an envelope that reshape/transpose the current input of the function depending on the control version.

CameronDevine commented 5 years ago

@CPCLAB-UNIPI, I just created a pull request (don4get/SIPPY#1) on @don4get's branch which fixes this issue, at least for the ARX_MIMO example.

don4get commented 5 years ago

Hello, thank you for the review and the fix! I just merged it, it should just work fine now.

don4get commented 5 years ago

Hello, do you have any news about the integration of this Merge Request? I would like to help you improve this library (adding tests, continuous integration features, etc.) but I don't know if the project is still active.

CPCLAB-UNIPI commented 5 years ago

@don4get we are merging the branches since everything has been validated. Further modifications will be applied to the main files. Thanks and sorry the the late response.