Closed pablokrupa closed 7 months ago
@VictorManuelGracia I have made the changes you suggested in your review. Thanks a lot!
I have also made some additional quality-of-life changes for you to have a look at:
spcies_function_name():
to spcies_snippet_name();
, which seems more intuitive. Also, these snippets are now located in spcies_root/snippets/
instead of types/functions/
.+utils/
folder to +sp_utils/
(sp
as short of spcies
). This is to avoid name conflicts with other toolboxes, etc. We want a name that is related to Spcies, instead of some generic name like +utils/
.laxMPC
. It did not work if time == false
.'type'
to 'formulation'
and 'subclass'
to 'submethod'
. I think these two new names are much more intuitive. I still allow 'type'
and 'submethod'
to be used (so that old code does not break), but I have included a warning stating that these names are now deprecated and will be removed in some future release (so that someone running old code will fix it). As a consequence of this change I have also changed types/
to formulation/
.The refactoring changes add a lot of changes to a lot of files, so I'm not asking for a new review because it will probably be a nightmare... But I would appreciate if you could share with me what you think of the changes and if you could confirm that this branch also works on your PC. I have run the tutorials on mine and they are working fine.
Finally, I have not updated the test files to these new refactor changes, because I know that you are working on them, so we can update them after you are done.
@VictorManuelGracia I added two more changes.
The first is that the documentation files are now markdown (.md
extension) to help readability if viewing the file directly (i.e., with the markdown formatting). Therefore, I have modified the files so that they follow markdown syntax. I have done it so that it also looks good if you print the documentation contents to Matlab's console window (i.e., using spcies('h', 'topic_name')
.
The second change is that I have cleaned up the root directory and made spcies()
the main entry point of the toolbox. I have moved the classes to a new classes/
directory and any function that is not intended for the user to directly call to +sp_utils/
. I have also added new cases to spcies()
. The idea is that the user will always use spcies('command')
to interact with the toolbox, instead of having to remember specific function names, such as spcies_gen_controller()
. Instead, the user will now use spcies('gen', args)
. This should make using the toolbox easier fro the user. In general we should encourage the use of this entry function. Therefore, I have also updated the tutorials so that they now use spcies('gen', args)
instead of spcies_gen_controller(args)
.
@VictorManuelGracia Another batch of changes. This time the changes are simply to update all the solvers with the new features we have been adding these last few months. In particular, now all solver:
nn_
, mm_
, etc.Also, I have changed the solution structures so that they are automatically named after save_name
. This is to avoid any possible naming issues in the future.
All in all, these last 12 commits are a general cleanup of the toolbox, so that all solvers are up to date with the latest features and conventions. Again, these updates affect a lot of lines of code. No need to look at them in detail.
@VictorManuelGracia I have modified the EADMM solver for MPCT so that matrices Q
and R
can be non-diagonal. I have added a new solver_option
to MPCT/EADMM called diag_QR
, which is set to true
by default. If this option is true and Q
and R
are diagonal, then the old solver is used (which uses the diagonal nature of Q
and R
to make perform one of the steps of the algorithm more efficiently). If either Q
or R
are non-diagonal or diag_QR == false
, then the new version of the solver is used (which should only be slightly slower). The addition of the diag_QR
option allows us to force the use of the "non-diagonal" solver even if Q
and R
are diagonal (for example when comparing against other solvers), but still default to the "diagonal-exploiting" solver by default (if Q
and R
are diagonal, of course).
Answering your latest comment.
- I still can use "spcies_gen_controller()", "or spcies_clear". I think that we should add a warning there, stating that the syntax is now updated to the new format "spcies()".
Yes, you can still use the old syntax. I have specifically made all these changes to maintain backwards compatibility with previous versions. This includes the use of 'type'
and 'subclass'
. If you use them you will get a warning saying that those names are now deprecated, but it will still work as before (internally it assigns 'type'
to 'formulation'
and 'subclass'
to 'submethod'
). As for spcies_gen_controller()
, it works just as before, but I want to direct the user to using spcies()
for everything, since this is much simpler and a cleaner user experience (in my opinion).
- The only real problem I see is the timer in Windows as I told you in one comment. It is quite easy to fix.
I have fixed it in a new commit (I hope). Please let me know if it is still not working.
- We need to add the rest of markdowns.
Yes. As I mention in a previous comment, the current doc files are for testing this new feature. There are still lots of files that need to be added. Particularly those related to the MPC formulations and optimization methods. My idea is to add them little by little during versions v0.4.x
. My idea is that version v0.5.0
is completely ready for third-party users (this requires a complete documentation of the toolbox and the testing suite).
- We also need to add the rest of functions (such as the ones used by FISTA with laxMPC) as snippets in the future.
Yes. This is also in the list of TODOs. I haven't done anything here because I want to have a close look at what parts of code can be shared between solvers before I do that. For instance, the code that solves the linear system with the banded diagonal Cholesky decomposition is the same in most of the solvers (the one that uses the Alpha
and Beta
variables). I would want to unify that between the solvers so that they all use the same snippet.
- I have tested the MPCT formulation with EADMM and works fine, either when Q and R are diagonal or not.
Great! Thanks.
- In my opinion, "method" may not be that clear to be understood by an external user. I don't know if you agree, but maybe "optimization_method" (maybe too long) or "opt_method", or even "optimizer", is more understandable. Consequently we would have to change "submethod" to something according to the change.
I wanted a word that was more or less clear but also not too long, so that using the toolbox does not require lines that are very long (as they are right now...). I think that the tutorials should make it quite clear in any case.
- Another thing I have thought of is the possibility of incorporating new subfolders inside "formulations/formulation_name/", such as "formulations/MPCT/templates" and "formulations/MPCT/ingredients", in order to distinguish for example which are the files of the templates, and which are the ones used to compute the ingredients.
- Another option to my last comment could be creating "formulations/MPCT/sub_method_name". For example, having "formulations/MPCT/ADMM/cs", "formulations/MPCT/ADMM/semiband" and "formulations/MPCT/EADMM". What I try to say is that maybe there are to many files inside "formulations/MPCT", for example.
Yes, I agree. There are a lot of files there, and it will only get more complicated as time goes on if we keep adding different variations of the MPCT formulation or different solvers. I think the naming convention helps in finding the files quickly. But yes, we could think of changing the structure to something like this. Then, in each of these folders we would have files that always have the same (and shorter) names. I will add it as an Issue so we keep it in mind. But for now I think there are more important things to focus on (mainly the user experience and documentation).
Also, I don't know how trivial this would be. For example, what if we add an MPCT variant which has soft constraints and is solved with ADMM. Should this be MPCT/ADMM/soft/
, or MPCT_soft/ADMM/
? What I mean is that maybe we need a more in-depth discussion about how we divide formulations/methods (and submethods) in general. And find a better toolbox structure to accommodate it.
I have tried t00_basic_tutorial.m and now works properly measuring times in Windows (in both Windows 10 and 11). You are right about snippets, we have to think of which functions should be snippets depending on the number of times the code is shared between different formulationsa and solvers. And I agree with you in terms of the internal organization of the folders, we have to be careful and use common sense.
I think that you can close the pull request. Nice job!
@VictorManuelGracia Sorry Victor, but I made a new change... The one about modifying how options are dealt with internally and how the user provides them to the toolbox.
The change I have made is to add a new class, called Spcies_options
, which deals with the toolbox options in a much cleaner and robust way. It should help us in clearly determining the options that each formulation/method consider and allow (which will in turn help in the documentation of the toolbox). In particular, the class defines the general options (as properties of the class). Then, each formulation-method-submethod defines has own def_options_formulation_method_submethod.m
where its specific options are given a default value. Spcies_options
uses these functions to generate those options inside the property Spcies_options.solver
. That is, Spcies_options.solver
is a structure that will contain the solver-specific options for the solver the user wants to generate.
Spcies_options
includes lots of methods for resetting options, changing formulation/method, and lots of checks to ensure that the options are set to reasonable values (except the ones in Spcies_options.solver
, which are not checked). It also has verbose (error or warnings) if you do something wrong. This should also help us a lot in the future to avoid bugs.
From the user's point of view, the main change is that solver_options
is now deprecated. I have maintained backwards compatibility, so you can still use it (a warning is thrown telling the user that solver_options
is now deprecated). However, the idea is that now the user will now add all his options as an options
structure (or as name-value parameters to spcies_gen_controller()
).
In the future we might want to consider changing this so that the user can first create the Spcies_options
structure using something like options = spcies('opt', [args])
and then call spcies('gen', options)
. However, for now I wanted to keep the changes from the user's perspective to a minimum for now (mainly because I want my old code to still work).
Finally, I would like to point out that this change has resulted in a lot of changes in the code-base. The examples work, but I would be surprised if I haven't broken some of the solvers. Please check any solvers you can easily check (the MPCT ones, for example) and let me know if any of them are broken. I will try and check some of the others (laxMPC
and equMPC
should be working fine). However, this highlights that what we really need if a solid testing feature, since otherwise making changes like this to the toolbox is too time consuming.
@VictorManuelGracia Thanks for the comments. I have fixed the error that you found. I want Spcies_options.solver
to be protected
to help us avoid bugs. In particular, having it as protected
avoid bugs such as opt.solver.roh = 1
, where roh
is a typo (rho
is correct). To modify values of Spcies_options.solver
the class provides the method Solver_options.set('key', value)
, which will only work if there already exists a field called key
in solver
. If you need to add a new field to solver
after the class is constructed, you can use the method Spcies_options.force('key', value)
, which acts like Solver_options.set('key', value), but will create a new field
solver.key` it if didn't exists already.
I have also solved bugs in the HMPC
, ellipHMPC
, ellipMPC
and MPCT
generator functions. Mainly bugs related to having missed some line that I had to modify due to the new use of Spcies_options
. I think all the solvers should be working now, so, unless you find something else, I will accept the pull request this afternoon.
This pull request adds two new features:
spcies_function_name();
by the contents of the file 'spcies_root/types/functions/name.extension', where.extension
is the extension of the file being generated. Currently, the new feature is only used to insert functions for measuring computation times in thecode_laxMPC_ADMM_C.c
andheader_laxMPC_ADMM_C.h
files. This feature can be very helpful for having a single source of code shared by various solvers (such as the time measuring functions or the function that solves the linear system of equations using the banded Cholesky decomposition).spcies('help', 'topic')
orspcies('h', 'topic')
, where'topic'
is the topic whose documentation will be displayed in the command window. The commandspcies('help')
will display the initial help page andspcies('help', 'topics')
will display the list of available topics. There are many missing topics, but a few have been included already (see 'spcies_root/docs/'). The idea is to keep adding new doc pages little by little.One of the new doc pages is
spcies('help', 'develop')
, which includes help for developers of the toolbox, including new git conventions, an explanation of the structure of the toolbox and basic guidelines for how to add a new solver.Also, the README.md file has been updated to reflect the new changes and to fix some aspects that were incorrect.