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

Full nosnoc pipeline for pss using the Stewart reformulation #90

Open
wants to merge 69 commits into
base: main
Choose a base branch
from

Conversation

apozharski
Copy link
Collaborator

No description provided.

+nosnoc/+dcs/stewart.m Outdated Show resolved Hide resolved
@nurkanovic
Copy link
Owner

The new structure is very clean and well-organized. Before we merge, I have a few comments to polish it further:

  • Maybe the files in different folders should not have the same name?
    E.g. we have several files Base, Stewart, etc.

  • Licence header missing in new functions

  • ocp_solver methods should have the same name formatting, e.g. get_x, get_u (instead of getU).

  • properties in an object that are not used should be removed? e.g. model.lsq_T, model.g_path should be removed if they are empty (I see that they are used to define "empty" path constraints, but it would be nice to have the output as light as possible). The corresponding flags are anyway created in model\Base

  • Similarly, having e.g. x_ref_val = zeros(...,...) is somewhat confusing, having them not at all in the output if not defined by the user would be nice.

  • rename ocp_solver.opts -> ocp_solver.problem_options , ocp_solver.solver_opts -> ocp_solver.solver_options

  • why are g_Stewart: [], sys_idx: [] always empty in .dcs Stewart?

  • replace h_res = ocp_solver.discrete_time_problem.w.h.res; by more readable syntax like h_res = ocp_solver.get('h'); other examples are:
    theta = ocp_solver.get_full('theta'), If a variable has a lower depth, then get and get_full should give the same resuls, e.g. h_res = ocp_solver.get_full('h') is same as h_res = ocp_solver.get('h');

  • there are some capitalizations to be discussed, e.g. nosnoc.ocp.solver(...), nosnoc.solver.options() look nicer then the last letter capitalized.
    problem_options = nosnoc.Options() or problem_options = nosnoc.options()?

  • In nosnoc, model, Base: % TODO: maybe a separate OCP class common to all model types? - please elaborate, why more classes?

  • g_comp_path -> how it is currently passed? as a n_path_comp x 2 matrix? How is then g_comp_path_fun handled?
    Should we maybe have a G_path and H_path instead, to me more consistent with our storing of complementarity constraints elsewhere?

  • the name g_switching and g_switching_fun are a bit confusing (I know that i coined them), but something more instructive could be used, e.g. g_lp_lagrangian
    or g_lp_stationarity

@apozharski
Copy link
Collaborator Author

apozharski commented May 24, 2024

A few responses here:

* Maybe the files in different folders should not have the same name?
  E.g. we have several files Base, Stewart, etc.

My opinion is this is fine. I prefer the clean-ness of model.Stewart (or even model.stewart as you suggest later) to something like model.StewartModel. I am not 100% set on this and if it causes development issues with the mainline matlab IDE then we can change this. it does not cause trouble with my emacs setup as it automatically tags each file with enough of its parent directories to make it unique, i.e. Stewart.m<+model> vs Stewart.m<+dcs>.

* Licence header missing in new functions

I think this is something to be tackled in the near future (pre v1.0?). I will write a quick script to automatically update the BSD licence header in the required .m files to include the current year and contain all the developers etc.

* ocp_solver methods should have the same name formatting, e.g. get_x, get_u (instead of getU).

Agreed

* properties in an object that are not used should be removed? e.g. model.lsq_T, model.g_path should be removed if they are empty (I see that they are used to define "empty" path constraints, but it would be nice to have the output as light as possible). The corresponding flags are anyway created in model\Base
* Similarly, having e.g. x_ref_val = zeros(...,...) is somewhat confusing, having them not at all in the output if not defined by the user would be nice.

What do you mean by "removed". There are two ways to do that for objects of classes: Actually removing via DynamicProps or just modifying the display to "hide" the unused properties via custom display functions. I use both to accomplish the results you see when you display vdx.Vector subclasses with dummy dynamic properties and some other custom work to display the relevant properties. This can absolutely be done but could you clarify what you mean.

* rename ocp_solver.opts  -> ocp_solver.problem_options , ocp_solver.solver_opts  -> ocp_solver.solver_options

I really prefer opts to options, but I will concede the point here.

* why are  g_Stewart: [], sys_idx: [] always empty in .dcs Stewart?

Because we use model.Stewart.g_ind for the first one and, sys_idx was something I thought I needed but is now vestigial. Will remove.

* replace h_res = ocp_solver.discrete_time_problem.w.h.res; by more readable syntax like h_res = ocp_solver.get('h'); other examples are:
  theta = ocp_solver.get_full('theta'), If a variable has a lower depth, then get and get_full should give the same resuls, e.g. h_res = ocp_solver.get_full('h') is same as h_res = ocp_solver.get('h');

No disagreement here.

* there are some capitalizations to be discussed, e.g. nosnoc.ocp.solver(...), nosnoc.solver.options() look nicer then the last letter capitalized.
  problem_options = nosnoc.Options() or problem_options = nosnoc.options()?

On purely aesthetics I agree, but at the same time this is useful to separate out classes from functions: Classes are capitalized where as function are not.

* In nosnoc, model, Base:         % TODO: maybe a separate OCP class common to all model types? - please elaborate, why more classes?

The idea here was to separate model definition from OCP definition, i,e. model only contains things necessary for dynamical definition of the system and an ocp class contains the necessary things for an OCP. I believe this idea was discussed and rejected previously.

* g_comp_path -> how it is currently passed? as a n_path_comp x 2 matrix? How is then g_comp_path_fun handled?
  Should we maybe have a G_path and H_path instead, to me more consistent with our storing of complementarity constraints elsewhere?

Yes currently it is passed that way. I have no strong opinions on storing it as G_path and H_path. Will re-implement that way.

* the name g_switching and g_switching_fun are a bit confusing (I know that i coined them), but something more instructive could be used, e.g. g_lp_lagrangian
  or g_lp_stationarity

g_lp_stationarity is fine with me I think.

@apozharski
Copy link
Collaborator Author

image
Current custom display implementation example.

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.

Implement new pipeline design for Stewart reformulation of PSS
2 participants