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

The function linearize() produces a Python file not written in a valid Python format. #8687

Closed
naibafomsare opened this issue Mar 12, 2022 · 20 comments
Assignees
Labels
COMP/OMC/Backend Issue and pull request related to the backend
Milestone

Comments

@naibafomsare
Copy link

Description

The linearize() function produces a file with information about the linearized model, that can be written in either Modelica, Python, Julia or Matlab format. In the case of Python, the file generated has errors.

The file is called linearized_model.py and contains a function. The fist error is that a semicolon is missing at the end of the first line. It should be def linearized_model(): rather than just def linearized_model().

The second error is that the format in which the A, B, C and D matrices are written, is not recognized by Python. It simply produces a syntax error. Although listening to the opinion of a Python expert is in order, it sounds natural to provide those matrices in a format that can be used directly in Python to define a space state model, for example, with the function control.ss. According to the documentation, such a function requires the matrix to be in an array_like or string format.

Steps to Reproduce

Linearize the model SpringPendulum.mo.txt by issuing the following commands on OMShell:

loadFile("SpringPendulum.mo") ;
clearCommandLineOptions() ;
setCommandLineOptions({"--linearizationDumpLanguage=python"}) ;
linearize(SpringPendulum) ;
getErrorString() ;

Then, open ipython and try to load the file with import linearized_model. You will get the syntax error message.

Expected Behavior

The function referred to above should be written in a valid Python format, and the state space matrices should be written in a form compatible with Python's control package.

A third observation is about how the information about the variable names and vector indices are provided. Those are not returned by the function, but they are rather written as comments with the hash symbol # in front of them. It would be far more useful to make this information available in the form of dictionaries, one for each of the state, input and output variables. The keys of the dictionaries could be the names of the variables, and the values, the indices of the variables within their corresponding vectors. Again, it would be good to ask a Python expert about this.

By the way, what are the variables

 # x_$STATESET1_x(1) = x(1);
 # x_$STATESET1_x(2) = x(2);
 # x_$STATESET1_x(3) = x(3);

What are their meaning in terms of the physical elements of the model? I want to calculate the eigenvectors of the system and knowing what they are would be useful.

Version and OS

  • OpenModelica Version: OpenModelica 1.18.1
  • OS: Ubuntu 20.04.3 LTS
@casella
Copy link
Contributor

casella commented Mar 14, 2022

@arun3688, you are familiar with Python. Could you please ask @kabdelhak where the code for this feature is, and try to fix it according to @naibafomsare's suggestions?

@casella casella added the COMP/OMC/Backend Issue and pull request related to the backend label Mar 14, 2022
@casella casella added this to the 1.20.0 milestone Mar 14, 2022
@casella
Copy link
Contributor

casella commented Mar 14, 2022

By the way, what are the variables

 # x_$STATESET1_x(1) = x(1);
 # x_$STATESET1_x(2) = x(2);
 # x_$STATESET1_x(3) = x(3);

The orientation of a rigid body in space is an SU(3) group. It cannot be fully represented by a fixed set of three variables (e.g. Euler's angles), because no matte how you choose them, there will always be a singular orientation. What OMC does is then to use dynamic state selection: it picks three potential states as actual states, but when they get close to singularity, it switches to some other choice. What you see are the names of those three generic dynamically selected states.

@vruge
Copy link
Member

vruge commented Mar 16, 2022

@adrpo
Copy link
Member

adrpo commented Mar 16, 2022

I remember we fixed the syntax issue for linearization but maybe we didn't or is an PR that was not merged?! Maybe Python 3 vs Python 2 issue?

I will check if I can reproduce this.

@bilderbuchi
Copy link

Maybe Python 3 vs Python 2 issue?

All of the syntax problems reported here or in the trac issue would also be present in Python 2.

@casella
Copy link
Contributor

casella commented Mar 17, 2022

The Matlab output, which was mentioned as broken in #6424 is fine, I used it in one of my courses and it worked fine. In fact, I also suggested the students that they could use Python, but nobody complained, so I guess all went with Matlab. We start feeding them Matlab when they are still toddlers, and that's the result :)

@arun3688, would you mind having a look?

@arun3688
Copy link
Contributor

@casella yes i will have a look into this

@arun3688
Copy link
Contributor

@casella I could reproduce the error, and also i found that linearize() API is not working in windows , i have created a ticket #8717

@casella
Copy link
Contributor

casella commented Mar 17, 2022

OK, do you think you can fix the syntax? That would be good already.

@arun3688
Copy link
Contributor

@casella I am currently working on the exporting unitDefinitions to ssp for OMSimulator, I will try to fix this as early as possible

@arun3688
Copy link
Contributor

@naibafomsare @casella The issue is fixed with this commit 809ed28 @naibafomsare you can test the changes from nighlty builds available from tomorrow (23-3-2022)

@bilderbuchi
Copy link

If such code is expected to be integrated into other Python code bases: One thing I noticed is that it's very customary to have scope-defining indentation levels be a multiple of 4 spaces (not 1), i.e. this way:

def linearized_model():
    # simple_test
    # der(x) = A * x + B * u
    # y = C * x + D * u
    bla = 4

There are some other linter-level inconsistencies that are probably too much effort to fix consistenly, but that one should be easy and will reduce the amount of linter noise significantly. :-)

@arun3688
Copy link
Contributor

@bilderbuchi that is true the python codes have default 4 spaces, but i see the current generated code has 2 spaces and not 1

@bilderbuchi
Copy link

Ah, I misread the diff, then, thought it was 1.

@naibafomsare
Copy link
Author

Thank you very much!

With the changes to the syntax in the linearized_model.py function, Python does not complain, and the matrices can be used with the control package to define a LTI system.

These changes are already useful. However, for larger systems, with several inputs, outputs and state variables, it would be good to provide the names of the variables in dictionaries, together with their indices within their vectors. Currently, what I do is to process the file linearized_model.mo as a text file, looking for the strings containing the variable names. This has worked so far, but makes the code more complicated, and it would break in case some conventions in writing the names in the file change.

@arun3688
Copy link
Contributor

arun3688 commented Mar 28, 2022

@naibafomsare The current generated python code structure looks like this

// "def linearized_model():
//   # simple_test
//   # der(x) = A * x + B * u
//   # y = C * x + D * u
//   n = 2 # number of states
//   m = 1 # number of inputs
//   p = 1 # number of outputs
//
//   x0 = [1.626558527192664, 2.380918053900121]
//   u0 = [0]
//
//   A = [[-2.887152375617477, -1.62655852935388],
// 	[-2.380918056675567, -2.388394731625707]]
//
//
//   B = [[0],
// 	[0]]
//
//
//   C = [[0, 0]]
//
//
//   D = [[4.007476581092785]]
//
//
//   # x_num_x(1) = x(1);
//   # x_num_x(2) = x(2);
//   # u_u = u(1);
//   # y_y = y(1);
//
//   return (n, m, p, x0, u0, A, B, C, D)

here

x0 - represents state vector
u0 - represents input vector
y0 - represents output vector 

the way to interpret, the comments # x_num_x(1) is the variables starting with

  1. x_ represents states
  2. u_ represents inputs
  3. y_ represents outputs

So in the current python generated code , we can generate new variables which contains the stateNames, InputNames and output names something like below

  stateNames = ["num_x[1]", "num_x[2]"]  ==> mapped to x0 = [1.626558527192664, 2.380918053900121]
  inputNames = ["u[1]"] ==> mapped to u0 = [0]
  outputNames = ["y[1]"] ===> mapped to y0=[]

So the final generated code looks something like below

def linearized_model():
  # simple_test
  # der(x) = A * x + B * u
  # y = C * x + D * u
  n = 2 # number of states
  m = 1 # number of inputs
  p = 1 # number of outputs
  x0 = [1.626558527192664, 2.380918053900121]
  u0 = [0]
  A = [[-2.887152375617477, -1.62655852935388],
	[-2.380918056675567, -2.388394731625707]]
  B = [[0],
	[0]]
  C = [[0, 0]]
  D = [[4.007476581092785]]
  # x_num_x(1) = x(1)
  # x_num_x(2) = x(2)
  # u_u = u(1)
  # y_y = y(1) 
  stateNames = ["num_x[1]", "num_x[2]"]
  inputNames = ["u[1]"]
  outputNames = ["y[1]"]
  return (n, m, p, x0, u0, A, B, C, D, stateNames, inputNames, outputNames)

and you can easily map the names with corresponding list, What do you think ?

@naibafomsare
Copy link
Author

@arun3688 The solution that you propose seems fine, and it's simpler: the indices, corresponding to each variable, are indicated by the place of the variable name within the list.

However, your example seems confusing, maybe because there's only one input and one output in the example:

# x_num_x(1) = x(1)
# x_num_x(2) = x(2)
# u_u = u(1)
# y_y = y(1) 
stateNames = ["num_x[1]", "num_x[2]"]
inputNames = ["u[1]"]
outputNames = ["y[1]"]

Shouldn't the inputNames and outputNames lists be

inputNames = ["u"]
outputNames = ["y"]

instead?

@arun3688
Copy link
Contributor

@naibafomsare , you are correct, That was an typing mistake

@casella
Copy link
Contributor

casella commented Mar 28, 2022

@naibafomsare, the issue of this ticket is fixed, so I'm closing it now. See #8770 for the new feature.

@casella casella closed this as completed Mar 28, 2022
@arun3688
Copy link
Contributor

@naibafomsare, the feature of generating different vars is implemented with this commit, 62abb40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP/OMC/Backend Issue and pull request related to the backend
Projects
None yet
Development

No branches or pull requests

7 participants