-
Notifications
You must be signed in to change notification settings - Fork 20
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
Made all count variable names consistent across the codebase. #94
Conversation
There were inconsistencies among different functions and the theory documentation regarding the variable names for the counts of different sets, e.g. number of states, number of unknown inputs, etc. This patch attempts to unify the language and add clarifications to different docstrings.
where the dimension of the unknown vectors are: | ||
|
||
- :math:`\mathbf{r}_u \in \mathbb{R}^q` | ||
- :math:`\mathbf{p}_u \in \mathbb{R}^r` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p and p_u have the same dimension r
docs/theory.rst
Outdated
@@ -156,12 +161,12 @@ Then, defining :math:`\mathbf{x}_i` to be: | |||
\mathbf{x}_i = [\mathbf{y}_i \quad \mathbf{r}_{ui} \quad \mathbf{p}_{ui}]^T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p_{ui} -> p_{u} ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p does not depend on time
docs/theory.rst
Outdated
optimization problem can formally be written as: | ||
|
||
.. math:: | ||
|
||
& \underset{\mathbf{x}_i \in \mathbb{R}^{(n + p)m + q}} | ||
& \underset{\mathbf{x}_i \in \mathbb{R}^{(n + q)N + r}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x_i has dimension n + q + r
it is the vector (y_1, r_u1, y_2, r_u2, ...p_u) that has dimension (n + q)N + r
@@ -178,14 +178,22 @@ def objective(self, free): | |||
|
|||
Parameters | |||
========== | |||
free : ndarray, (n * N + m * M + q, ) | |||
free : ndarray, (n*N + q*N + r, ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok (here and below)
- n : number of unknown state trajectories | ||
- q : number of unknown input trajectories | ||
- r : number of unknown parameters | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a description of 'numinstance'
- N : number of collocation nodes | ||
- n : number of states | ||
- m : number of input trajectories | ||
- p : number of parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory.rst parameters and unknown parameters have the same dimension: r
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review done. Please check dimensionality of p, p_u and x_i.
Merging. Thanks! |
There were inconsistencies among different functions and the theory
documentation regarding the variable names for the counts of different
sets, e.g. number of states, number of unknown inputs, etc. This patch
attempts to unify the language and add clarifications to different
docstrings.