ICB-DCM / PESTO

PESTO: Parameter EStimation TOolbox, Bioinformatics, btx676, 2017.
https://doi.org/10.1093/bioinformatics/btx676
BSD 3-Clause "New" or "Revised" License
26 stars 12 forks source link

getParameterSamples cannot be called with PestoOptions instance #59

Closed dweindl closed 7 years ago

dweindl commented 7 years ago

Due to getParameterSamples.m 118-120

   opt.number = parameters.number;
   opt.min    = parameters.min;
   opt.max    = parameters.max;

getParameterSamples cannot be called with PestoOptions instance, because these members do not exist. (see runPestoTests)

(why) is it required to copy these values into opt?

BBallnus commented 7 years ago

This is currently required, because sampling methods need boundaries and do not get the parameter struct as input so far. This basically makes the sampling methods currently function as standalone without the need for other PESTO functions or structures.

This discussion is very similar to the discussion about opt.objOutNumber -> parameters.objOutNumber from a couple of days ago, where we decided to not rearrange the option, since Paul did not want to toss the entire parameter struct into some sub-function. I personally do not want to make the sampling routines more dependent than necessary.

dweindl commented 7 years ago

Yes, it refers to the previous discussion. Still, we should have a unified interface now.

What is wrong with changing

function res = performPT( logPostHandle, opt )

to

function res = performPT( logPostHandle, par, opt )

?

BBallnus commented 7 years ago

I think the interface is unified, since the user only interacts with

function parameters = getParameterSamples(parameters, objFkt, opt)

and not with performPT, which is private.

Your suggested change is possible indeed. Yet I'm not convinced it is necessary. :)

P.S. Gonna change it in a couple of minutes

dweindl commented 7 years ago

I think the interface is unified, since the user only interacts with function parameters = getParameterSamples(parameters, objFkt, opt)

Not exactly, since opt is of a different type than for all other functions.

BBallnus commented 7 years ago

Gonna upload an adapted version, please let me know if this works for you

dweindl commented 7 years ago

Something is wrong with checkSamplingOptions

    Error using checkSamplingOptions (line 21)
    Please specify the type of objective function, e.g. opt.obj_type = "log-posterior"

    Error in getParameterSamples (line 118)
       checkSamplingOptions(parameters,opt);

    Error in ConversionReactionTest/testSamplingSingleChainAM (line 109)
                parameters = getParameterSamples(testCase.parameters, testCase.objectiveFunction, optionsMultistart);

... although

K>> opt.obj_type

ans =

log-posterior

To reproduce, run runPestoTests

BBallnus commented 7 years ago

This is not reproducible for me - neither by calling the example directly nor calling it via runPestoTests

dweindl commented 7 years ago

@FFroehlich Does runPestoTests complete successfully for you?

FFroehlich commented 7 years ago

runPestoTests runs fine for me.

dweindl commented 7 years ago

(On develop?)

For me in MATLAB Version: 9.0.0.341360 (R2016a) on Linux it does not.

FFroehlich commented 7 years ago

I get the same:

Error occurred in ConversionReactionTest/testSamplingSingleChainAM and it did not run to completion.

---------
Error ID:
---------
''

--------------
Error Details:
--------------
Error using checkSamplingOptions (line 21)
Please specify the type of objective function, e.g. opt.obj_type = "log-posterior"
Error in getParameterSamples (line 118)
   checkSamplingOptions(parameters,opt);

Error in ConversionReactionTest/testSamplingSingleChainAM (line 109)
            parameters = getParameterSamples(testCase.parameters, testCase.objectiveFunction, optionsMultistart);
BBallnus commented 7 years ago

Now I get it! Sorry for the confusion, I was still talking about RunTestExamples, while you guys were talking about runPestoTests. However, I get a different error:

runPestoTests Running ConversionReactionTest

================================================================================ Fatal assertion failed in ConversionReactionTest/testObjectiveFunction. Test run aborted.

----------------
Test Diagnostic:
----------------
Likelihood function result is wrong.

---------------------
Framework Diagnostic:
---------------------
fatalAssertEqual failed.
--> The values are not equal using "isequaln".
--> The error was not within absolute tolerance.
--> Failure table:
                 Actual             Expected              Error              RelativeError         AbsoluteTolerance  
            ________________    ________________    __________________    ____________________    ____________________

            32.2755461526847    32.2755461526847    7.105427357601e-15    2.20148942607745e-16    2.22044604925031e-16

Actual double:
      32.275546152684697
Expected double:
      32.275546152684690

------------------
Stack Information:
------------------
In C:\Users\....\tests\ConversionReactionTest.m (ConversionReactionTest.testObjectiveFunction) at 45

================================================================================ Error using ConversionReactionTest/testObjectiveFunction (line 45) Fatal assertion failed.

paulstapor commented 7 years ago

I'm currently implementing a class PestoSamplingOptions. This will make this at least mostly uniform...

BBallnus commented 7 years ago

This issue should be fixed, since Paul and I shifted some code to a sampling options class.

BBallnus commented 7 years ago

After Reading Fabian comment in #43, I decided to reopen it until we have updated the Master-Branch.