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

Index refactor #26

Merged
merged 18 commits into from
Dec 19, 2022
Merged

Index refactor #26

merged 18 commits into from
Dec 19, 2022

Conversation

apozharski
Copy link
Collaborator

@apozharski apozharski commented Dec 15, 2022

Refactoring index definitions. Very work in progress.

@apozharski apozharski changed the title WIP: Index refactor Index refactor Dec 15, 2022
@apozharski apozharski assigned apozharski and unassigned apozharski Dec 15, 2022
Copy link
Collaborator

@FreyJo FreyJo left a comment

Choose a reason for hiding this comment

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

2 minor comments.
LGTM

problem.g = {};
problem.lbg = [];
problem.ubg = [];
problem.ind_g_clock_state = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just checked out of curiosity, ind_g_clock_state seems to never be used.
Can be removed later of course.

src/matlab/create_nlp_nosnoc.m Show resolved Hide resolved
Copy link
Owner

@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.

a short description for add_variable (e.g., what are possible values for type)
would be useful, even though it is almost apparent what needs to be done.

Copy link
Owner

@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.

To make create_nlp_nosnoc shorter, I suggest to make some small stupid functions, e.g.,
create_empty_problem for initializing the problem struct

Copy link
Owner

@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.

we often use (which was my initial mistake) cells for stacking functions and variables, g = {problem.g{:}, g}, and do vertcat(g(:)) at the end. we could immediately just do
problem.g = [problem.g, g], right @FreyJo ?

@FreyJo
Copy link
Collaborator

FreyJo commented Dec 19, 2022

we often use (which was my initial mistake) cells for stacking functions and variables, g = {problem.g{:}, g}, and do vertcat(g(:)) at the end. we could immediately just do problem.g = [problem.g, g], right @FreyJo ?

Right, it should be possible to just do vertcat in the new helper functions.
But, as Anton said this is one cleanup step and there are surely more to come.

@FreyJo
Copy link
Collaborator

FreyJo commented Dec 19, 2022

a short description for add_variable (e.g., what are possible values for type) would be useful, even though it is almost apparent what needs to be done.

Personally, I would not put comments in such a self explaining function, which is anyway just a helper function for developers, and not part of the front-end.

@nurkanovic nurkanovic merged commit 7f8c11b into main Dec 19, 2022
@nurkanovic nurkanovic deleted the index-refactor branch December 19, 2022 11:19
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.

None yet

3 participants