csu-hmc / opty

A library for using direct collocation in the optimization of dynamic systems.
http://opty.readthedocs.io
Other
86 stars 20 forks source link

Mistake in parse_free docstring #114

Closed tjstienstra closed 4 months ago

tjstienstra commented 4 months ago

The opty.utils.parse_free docstring states:

def parse_free(free, n, q, N):
    """Parses the free parameters vector and returns it's components.

    Parameters
    ----------
    free : ndarray, shape(n*N + q*N + r)
        The free parameters of the system.
    n : integer
        The number of states.
    q : integer
        The number of free specified inputs.
    N : integer
        The number of time steps.

    Returns
    -------
    states : ndarray, shape(n, N)
        The array of n states through N time steps.
    specified_values : ndarray, shape(r, N) or shape(N,), or None
        The array of r specified inputs through N time steps.
    constant_values : ndarray, shape(q,)
        The array of q constants.

    """

However, the opty.direct_collocation.ContraintCollocator docstring states:

    """
    ...

    - N : number of collocation nodes
    - n : number of states
    - m : number of input trajectories
    - p : number of parameters
    - q : number of unknown input trajectories
    - r : number of unknown parameters
    - o : number of instance constraints
    - nN + qN + r : number of free variables
    - n(N - 1) + o : number of constraints

    """"

The variables used in these two docstrings do not match. The stated sizes of the return values in opty.utils.parse_free are wrong and should be:

def parse_free(free, n, q, N):
    """
    ...

    Returns
    -------
    states : ndarray, shape(n, N)
        The array of n states through N time steps.
    specified_values : ndarray, shape(q, N) or shape(N,), or None
        The array of q specified inputs through N time steps.
    constant_values : ndarray, shape(r,)
        The array of r constants.

    """

Is this correct? If so, I'll open a PR to correct this mistake.

moorepants commented 4 months ago

Yes, seems like they should be switched (if the constraintcollocator list is correct).