8000 Full nosnoc pipeline for pss using the Stewart reformulation by apozharski · Pull Request #90 · nosnoc/nosnoc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Full nosnoc pipeline for pss using the Stewart reformulation #90

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 85 commits into from
Jun 18, 2024
Merged

Full nosnoc pipeline for pss using the Stewart reformulation #90

merged 85 commits into from
Jun 18, 2024

Conversation

apozharski
Copy link
Member

No description provided.

@apozharski apozharski force-pushed the new-design-stewart branch from 9db9e26 to a8b70dc Compare May 13, 2024 12:31
@apozharski apozharski linked an issue May 13, 2024 that may be closed by this pull request
Implement new pipeline design for Stewart reformulation of PSS #92
Closed
@apozharski apozharski force-pushed the new-design-stewart branch from 91b252a to 8621312 Compare May 14, 2024 08:06
@apozharski apozharski force-pushed the new-design-stewart branch from 8621312 to 88229d4 Compare May 14, 2024 08:09
@apozharski apozharski assigned apozharski and unassigned apozharski May 15, 2024
@apozharski apozharski requested a review from nurkanovic May 15, 2024 08:54
nurkanovic
nurkanovic reviewed May 15, 2024
View reviewed changes
@nurkanovic
Copy link
Member

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
Member Author
apozharski commented May 24, 2024 •
edited
Loading

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
Member Author

image
Current custom display implementation example.

@nurkanovic
Copy link
Member

My previous comments where addressed in a satisfactory way. I looked now in detail through all new files, and we are close to merge the PR.
I added couple of comments in the code to help refresh ones memory when looking at it with some time distance.

Before we finish, I have few rather small things left:

  1. integrator.simulate() should have the same verbose as the previous integrator had (e.g. how much in time it advanced) - it was useful for debugging.

  2. it is a bit dissatisfying that integrator.stats stores the stats only of the last integration step. sometimes it is useful to plot the cpu time of each integration step (this can be a separate issue)

  3. the integrator does not have anymore what used to be result.extended.t_grid, i.e. the time grid that has the collocation time points in between. this is useful for visualization, and debugging.

  4. it seems redundant to have both ocp_solver.get_x() and ocp_solver.get('x') if they get the same result. I would keep only the latter one.

  5. What is the purpose of ocp_solver.getResults() (get_results() would be a more consistent name)

  6. ocp_solver.dcs should also print x (and possibly u) in the variables.

  7. % time varying parameters
    p_time_var
    p_time_var_val
    % params
    p_time_var_stages
    % all params
    p
    Please comment in the code the differences. Not clear why p_time_var_stages has no _val and what is the difference to p_time_var

  8. It would be useful to have also upper bounds on G_path and H_path (but can be a different small Issue/PR)

  9. Step equilibration in discrete_time_problem/Stewart , obj.w.lambda_theta(ii,2:opts.N_finite_elements(ii)) = {{'lambda_theta', dims.n_theta},0,inf}; obj.w.lambda_lambda(ii,2:opts.N_finite_elements(ii)) = {{'lambda_lambda', dims.n_lambda},0,inf}; the variable names are not instructive.

  10. Please justify why x_0 = obj.w.x(0,0,opts.n_s) and not x_0 = obj.w.x(0,0,1) or x_0 = obj.w.x(0,0,0) (also while defining it)

  11. StepEquilibrationMode.mlcp might have a more instructive/explicit name

@apozharski
Copy link
Member Author

My previous comments where addressed in a satisfactory way. I looked now in detail through all new files, and we are close to merge the PR. I added couple of comments in the code to help 6D40 refresh ones memory when looking at it with some time distance.

Before we finish, I have few rather small things left:

1. integrator.simulate() should have the same verbose as the previous integrator had (e.g. how much in time it advanced) - it was useful for debugging.

Addressed

2. it is a bit dissatisfying that integrator.stats stores the stats only of the last integration step. sometimes it is useful to plot the cpu time of each integration step (this can be a separate issue)

Addressed

3. the integrator does not have anymore what used to be result.extended.t_grid, i.e. the time grid that has the collocation time points in between. this is useful for visualization, and debugging.

Addressed

4. it seems redundant to have both ocp_solver.get_x() and ocp_solver.get('x') if they get the same result. I would keep only the latter one.

Addressed

5. What is the purpose of ocp_solver.getResults()  (get_results() would be a more consistent name)

Removed, there due to old design idea.

6. ocp_solver.dcs should also print x (and possibly u) in the variables.

Added. I think all the display code will end up being a bit of a chore to update but that is the life of providing a nice user experience.

7. % time varying parameters
   p_time_var
   p_time_var_val
   % params
   p_time_var_stages
   % all params
   p
   Please comment in the code the differences. Not clear why p_time_var_stages has no _val and what is the difference to p_time_var

Frankly p_time_var_stages is never used anywhere except as an excuse to generate all of the symbolics at model time. I am not sure why this would be useful unless you want to for some reason have time varying params affect other time intervals which would be a horrible hack. As such this was removed.

8. It would be useful to have also upper bounds on G_path and H_path (but can be a different small Issue/PR)

Agree but imo this is a minor thing and should indeed be a separate issue.

9. Step equilibration in discrete_time_problem/Stewart , obj.w.lambda_theta(ii,2:opts.N_finite_elements(ii)) = {{'lambda_theta', dims.n_theta},0,inf};  obj.w.lambda_lambda(ii,2:opts.N_finite_elements(ii)) = {{'lambda_lambda', dims.n_lambda},0,inf}; the variable names are not instructive.

I don't have better instructive names that aren't significantly longer. And while we are no longer limited to 80 columns by punch cards I still think a shorter name that is accurate (lambda_lambda is the multiplier for the lambda KKT conditions) is far more readable than something longer.

10. Please justify why x_0 = obj.w.x(0,0,opts.n_s) and not x_0 = obj.w.x(0,0,1) or x_0 = obj.w.x(0,0,0) (also while defining it)

There are several (one major) ergonomic reasons to do this, as well as a philosophical one:
It allows clean extraction from vdx.Variable as it lets you do problem.w.var(:,:,opts.n_s).res which gets the full trajectory including the initial point.
Further on a philosophical level the initial position is the end point of some previous process.

To me the ergonomic reasons are sufficient to overrule basically anything as making the extremely common task of data extraction take extra lines for no reason simply increases the static friction one must overcome when extending the code.

11. StepEquilibrationMode.mlcp might have a more instructive/explicit name

We could perhaps call it linear or linear_complementarity. Though those really don't give away much more about what is going on.

nurkanovic
nurkanovic approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member
@nurkanovic nurkanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments are addressed - ready to be merged! GJ.

@nurkanovic nurkanovic merged commit 746fb16 into main Jun 18, 2024
1 check passed
@apozharski apozharski deleted the new-design-stewart branch January 27, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nurkanovic nurkanovic nurkanovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement new pipeline design for Stewart reformulation of PSS
2 participants
@apozharski @nurkanovic
0