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

GNSF 2.0 #325

Merged
merged 30 commits into from Sep 7, 2018

Conversation

Projects
None yet
3 participants
@FreyJo
Member

FreyJo commented Aug 24, 2018

Hey guys,

this PR additionally to #319 contains the following changes:

  • I generalized the GNSF structure further, allowing more linear dependencies in the linear output type system and also introducing a split of z, such that it can be partly modelled by the linear output system part. Now the GNSF structure reads as
    this: gnsf_structure.pdf
  • Updated sim_gnsf and Matlab structure detection accordingly
  • Regenerated GNSF models accordingly
  • Cleaned the automatic model generation in Matlab and documented it in the wiki:
    How to: Dynamic System Models and Integrators in acados

My intention is that this will be merged instead of #319 .

FreyJo added some commits Aug 19, 2018

crane_dae_model: added more dependencies for testing; adjusted sim_te…
…st_dae tolerance due to increased nonlinearity
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 24, 2018

Codecov Report

Merging #325 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   77.51%   77.73%   +0.22%     
==========================================
  Files          68       68              
  Lines       12860    12988     +128     
==========================================
+ Hits         9968    10096     +128     
  Misses       2892     2892

codecov bot commented Aug 24, 2018

Codecov Report

Merging #325 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   77.51%   77.73%   +0.22%     
==========================================
  Files          68       68              
  Lines       12860    12988     +128     
==========================================
+ Hits         9968    10096     +128     
  Misses       2892     2892
@dkouzoup

This comment has been minimized.

Show comment
Hide comment
@dkouzoup

dkouzoup Sep 5, 2018

Member

@FreyJo Can you pull the latest master here to only see the latest differences? (At least I think it works that way)

Member

dkouzoup commented Sep 5, 2018

@FreyJo Can you pull the latest master here to only see the latest differences? (At least I think it works that way)

FreyJo added some commits Sep 5, 2018

@dkouzoup

I guess @FreyJo is the most suitable person to approve this PR :P Let us know when it is ready to merge from your side. Great job!

if (inString == "IRK") return 1e-4;
if (inString == "GNSF") return 1e-4;
if (inString == "IRK") return 3e-4;
if (inString == "GNSF") return 3e-4;

This comment has been minimized.

@dkouzoup

dkouzoup Sep 6, 2018

Member

why?

@dkouzoup

This comment has been minimized.

@FreyJo

FreyJo Sep 6, 2018

Member

I changed the example used in the test, adding more nonlinearities to the algebraic equations. Thus, I had to adjust the accuracy required to pass the test.

@FreyJo

FreyJo Sep 6, 2018

Member

I changed the example used in the test, adding more nonlinearities to the algebraic equations. Thus, I had to adjust the accuracy required to pass the test.

This comment has been minimized.

@dkouzoup

dkouzoup Sep 6, 2018

Member

Ok! Is it not possible to change the integrator options to give higher accuracy solutions? Even 1e-4 seems a bit high for a test.

@dkouzoup

dkouzoup Sep 6, 2018

Member

Ok! Is it not possible to change the integrator options to give higher accuracy solutions? Even 1e-4 seems a bit high for a test.

This comment has been minimized.

@FreyJo

FreyJo Sep 6, 2018

Member

Not really.
The output for algebraic variables gets computed without performing an additional Newton-iteration, but by evaluating the polynomial through the values of the first stage at the starting time of the simulation.
Other things could be implemented, but I think are not priority...

@FreyJo

FreyJo Sep 6, 2018

Member

Not really.
The output for algebraic variables gets computed without performing an additional Newton-iteration, but by evaluating the polynomial through the values of the first stage at the starting time of the simulation.
Other things could be implemented, but I think are not priority...

@FreyJo

This comment has been minimized.

Show comment
Hide comment
@FreyJo

FreyJo Sep 6, 2018

Member

Yes for me this is ready to be merged! Also fixed the memory thing for which you pinged me yesterday.

Member

FreyJo commented Sep 6, 2018

Yes for me this is ready to be merged! Also fixed the memory thing for which you pinged me yesterday.

@dkouzoup

This comment has been minimized.

Show comment
Hide comment
@dkouzoup

dkouzoup Sep 6, 2018

Member

Yeah I noticed! Which will force me to resolve a conflict but I forgive you :P

I will leave the merge to @giaf then, maybe you can meet in person tomorrow.

Member

dkouzoup commented Sep 6, 2018

Yeah I noticed! Which will force me to resolve a conflict but I forgive you :P

I will leave the merge to @giaf then, maybe you can meet in person tomorrow.

@giaf

This comment has been minimized.

Show comment
Hide comment
@giaf

giaf Sep 7, 2018

Collaborator

@dkouzoup today we discussed a little about initialization of the integrator variables. Currently the initial values for xdot and z are passed in sim_in. But this does not scale to GNSF, since this requires the initialization of other quantities which are only present in GNSF (and therefore not in sin_in, whose definition is common to all integrators).

What would be a good solution here? Maybe move all initializations to memory, which is different for each integrator?

Collaborator

giaf commented Sep 7, 2018

@dkouzoup today we discussed a little about initialization of the integrator variables. Currently the initial values for xdot and z are passed in sim_in. But this does not scale to GNSF, since this requires the initialization of other quantities which are only present in GNSF (and therefore not in sin_in, whose definition is common to all integrators).

What would be a good solution here? Maybe move all initializations to memory, which is different for each integrator?

@dkouzoup

This comment has been minimized.

Show comment
Hide comment
@dkouzoup

dkouzoup Sep 7, 2018

Member

Hmmm good question. In my opinion, I would keep the initialization of variables where the variables belong. If we have xdot and z in sim_in, I would initialize them there, and variables that are located in the GNSF memory would be init. upon the creation of the GNSF memory, no?

Member

dkouzoup commented Sep 7, 2018

Hmmm good question. In my opinion, I would keep the initialization of variables where the variables belong. If we have xdot and z in sim_in, I would initialize them there, and variables that are located in the GNSF memory would be init. upon the creation of the GNSF memory, no?

@FreyJo

This comment has been minimized.

Show comment
Hide comment
@FreyJo

FreyJo Sep 7, 2018

Member

@dkouzoup The integration variables can not be initialized when the memory is created (with anything else but zeros), as no values enter the memory_create function, which has the following input:
void *config, void *dims_, void *opts_, void *raw_memory
Also, xdot and z are now part of sim_in to implement initialization for sim_irk, but I am not sure if they naturally belong there. Or if initialization should always happen through the memory. For multiple runs it should happen through the memory internally I guess, but the in the first call I am not sure.
Anyway I am fine with how it is now - and could soon add this internal initialization, copying the last result for the integration variables in the memory and take it as new initial guess.

Member

FreyJo commented Sep 7, 2018

@dkouzoup The integration variables can not be initialized when the memory is created (with anything else but zeros), as no values enter the memory_create function, which has the following input:
void *config, void *dims_, void *opts_, void *raw_memory
Also, xdot and z are now part of sim_in to implement initialization for sim_irk, but I am not sure if they naturally belong there. Or if initialization should always happen through the memory. For multiple runs it should happen through the memory internally I guess, but the in the first call I am not sure.
Anyway I am fine with how it is now - and could soon add this internal initialization, copying the last result for the integration variables in the memory and take it as new initial guess.

@dkouzoup

This comment has been minimized.

Show comment
Hide comment
@dkouzoup

dkouzoup Sep 7, 2018

Member

Ok I did not get that we need to initialize these fields to user-specified values. Initialization upon creation only makes sense if this is always fixed (e.g. zeros, or an identity matrix).
In the case you describe I suppose that setters should be implemented to edit things in the memory after creation

Member

dkouzoup commented Sep 7, 2018

Ok I did not get that we need to initialize these fields to user-specified values. Initialization upon creation only makes sense if this is always fixed (e.g. zeros, or an identity matrix).
In the case you describe I suppose that setters should be implemented to edit things in the memory after creation

@FreyJo

This comment has been minimized.

Show comment
Hide comment
@FreyJo

FreyJo Sep 7, 2018

Member

Yes just wanted to clarify this.
Lets talk about this next week.
Anyway, I think the state now is quite good, especially I dont expect the GNSF stuff that this is PR is about to change. Thus I suggest to merge it.

Member

FreyJo commented Sep 7, 2018

Yes just wanted to clarify this.
Lets talk about this next week.
Anyway, I think the state now is quite good, especially I dont expect the GNSF stuff that this is PR is about to change. Thus I suggest to merge it.

@giaf

This comment has been minimized.

Show comment
Hide comment
@giaf

giaf Sep 7, 2018

Collaborator

ok let's do it then

Collaborator

giaf commented Sep 7, 2018

ok let's do it then

@giaf giaf merged commit 17ee705 into acados:master Sep 7, 2018

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 77.73% (target 0%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment