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

Make ode45reg work in reverse time #18

Closed
pgagarinov opened this issue May 3, 2015 · 7 comments
Closed

Make ode45reg work in reverse time #18

pgagarinov opened this issue May 3, 2015 · 7 comments
Assignees

Comments

@pgagarinov
Copy link
Member

Deadline - 27th of October, by that time both changes and tests should be merged into "master" branch

As opposed to built-in ode45 function gras.ode.ode45reg cannot handle reverse time just yet. The solution is to replace t with -t using anonymous functions inside ode45reg and then apply fliplr just before returning the output arguments. Once this is done one needs to write a test inside products+gras+ode+test+mlunit\SuiteOde45Reg.m that iterates through a few different systems (functions for right hand side of ode equation) and compares that a solution in a direct time on [0,T] coincides with a flipped solution of the system where we substitute t with T-t. Comparison should be done with modgen.common.absrelcompare (see how existing tests in SuiteOde45Reg test case work).

I think the best way would be doing this as the following sequence of steps:

  1. rename ode45reg into ode45regdir where "dir" means "direct time"
  2. implementing a wrapper function ode45reg that
    a) if tSpanVec is in forward time - ode45regdir directly OR
    b) if tSpanVec is in opposite time - adjusts input arguments (timeVec,fOdeReg,tSpanVec,y0Vec as mentioned above), then calls ode45regdir and finally adjusts the output arguments back (this includes interpObj of gras.ode.VecOde45RegInterp class - see step 3).
  3. Define an interface gras.ode.IVecOdeReg that defines "evaluate" method with the same signature as in VecOde45RegInterp and make VecOde45RegInterp define this interface. Also please add getTStart and getTEnd methods that returns self.tBegin and self.tFinal.
  4. Create a new class VecOdeRegInverseTimeWrapper that implements the same interface. A constructor of this class should accept an object of VecOde45RegInterp class and store it in its private field "interpObj". "evaluate" method should call self.interpObj.evaluate and then adjust the outputs so that they correspond to a forward time again. This makes VecOdeRegInverseTimeWrapper a thin wrapper that just changes a direction of time. When making a transformation you will have to call self.interpObj.getTStart() and self.interpObj.getTEnd() to make a time direction adjustment (T-t -> t requires knowing T=tEnd-tStart).
  5. Make ode45reg return an object of VecOdeRegInverseTimeWrapper class in case tSpanVec corresponds to an inverse time.
  6. Write a few additional tests next to already existing tests to make sure that everything works as expected.
@arturlu
Copy link
Contributor

arturlu commented Jan 16, 2016

@pgagarinov Should such behaviour affects VecOde45RegInterp.evaluate() method (instance of VecOde45RegInterp is returned from ode45reg as 4th output) ?

@pgagarinov
Copy link
Member Author

I hope a more detailed description above answers this question.

pgagarinov pushed a commit that referenced this issue Jan 16, 2016
BugFix: minor cosmetic change
arturlu added a commit that referenced this issue Jan 16, 2016
Enhancement: ode45reg accepts reverse time tspan argument now
arturlu added a commit that referenced this issue Jan 16, 2016
Enhancement: ode45reg accepts reverse time tspan argument now
arturlu added a commit that referenced this issue Jan 16, 2016
Enhancement: gras.ode.ode45reg now can return interpolator which supports reverse time vectors
@arturlu
Copy link
Contributor

arturlu commented Jan 16, 2016

@pgagarinov Thanks for the reply.
I have done.

BTW getTStart/getTEnd methods are not strictly required, because we can do the time convertation by using only t'=-t substitution, T-t substitution with T offset is not required.

@pgagarinov
Copy link
Member Author

  1. Why don't you transform yOutMat and dyRegMat after they are returned by ode45regdir? From my perspective you need to do this to restore an original order of elements in those matrices. Also please check correctness of your tests - they should fail right now (until you fix the problem), otherwise they are useless.

  2. Please make all variable names in the tests you implemented match our naming convention.

  3. Please move bodies of all subfunctions functions in testInverseTspan to the end of the test so that we do not have a code of the test mixed up with the subfunction code.

I rebooted the Jenkins server so now the tests in your branch should be started. Please monitor the failing tests in the corresponding email group.

arturlu added a commit that referenced this issue Jan 17, 2016
Code of ode45 test is updated according to coding policy and its quality was improved
@arturlu
Copy link
Contributor

arturlu commented Jan 17, 2016

@pgagarinov

  1. You can formally check that only t'=-t substitution and following sign change of time are required to handle inverse time use case. I have checked the implemented code with native MATLAB ode45 implementation - reordering is not required.
    2-3) Done.

@pgagarinov
Copy link
Member Author

  1. Ok, makes sense.
  2. Please either get rid of
            if ~exist('CMP_TOL', 'var')
                CMP_TOL = 1e-8;
            end

or

use nargin<7 instead of ~exist

Once you are done with 4) you can merge to master using the following procedure:


Then wait for the test results - if all tests pass (and only if they pass) - merge to master.
When merging - please follow this algorithm very carefully

  1. Switch to master
  2. Merge -> from -> origin/remotes/your branch (using local branch can dangerous because it can contain unpushed changes)
  3. Important !!! Set "no fast forward flag". Press OK
  4. DO NOT PUSH JUST YET. Right click and open log. It should look like "petlya".
  5. If eveything is ok - press push.
  6. Wait for the test results for updated master. If everything ok - switch to the next task.

@pgagarinov
Copy link
Member Author

Instead of assigning a single big task as a main task I assigned 3 small tasks which is fair. Please start with task #46

arturlu added a commit that referenced this issue Jan 17, 2016
Code of ode45 test is updated according to coding policy and its quality was improved
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

No branches or pull requests

2 participants