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

Open Pesto-Issues for Release #62

Closed paulstapor closed 7 years ago

paulstapor commented 7 years ago

A list of things which I think are necessary to fix.

...to be continued, but these are the first which came to my mind

dweindl commented 7 years ago

So to summarize, except for these two coding tasks,

  • Make Pesto also run when no PestoOptions, but a struct with the same fields (or little changes like things were in the old versions, e.g. options.fmincon instead of options.localOptimizerOptions) is provided (for backward campatibility) ---> who ever has time, maybe use old setDefault-function for a starting point
  • The same should hold true more or less for Sampling options (backward compatibility) in the best case, but this I see as optional

there is only testing to be done for now, right?

FFroehlich commented 7 years ago

please use - [ ] to make bullet points checkable. makes tracking of what is done and what not easier and avoids excessive email spam. collectResults was fine for me.

FFroehlich commented 7 years ago

for the pesto options thing you can copy the following 5 lines from AMICI: https://github.com/ICB-DCM/AMICI/blob/master/%40amioption/amioption.m#L107 (this makes you have all the default options and internal checks directly available for the import)

dweindl commented 7 years ago

for the pesto options thing you can copy the following 5 lines from AMICI: https://github.com/ICB-DCM/AMICI/blob/master/%40amioption/amioption.m#L107 (this makes you have all the default options and internal checks directly available for the import)

The whole thing is already based on the amioption class. What needs to be changed is the argument check in the other functions. I will do that.

--> #64

dweindl commented 7 years ago

Anybody feeling responsible for any of the tasks, please open a new issue (one per task) and assign yourself.

dweindl commented 7 years ago

Priority should be fixing these issues: https://github.com/ICB-DCM/PESTO/issues?q=is%3Aopen+is%3Aissue+milestone%3A%22Release+1.0%22+label%3Abug

The other points are rather vague and will not cause any errors. Although they are nice to have... I created the release-1.0 branch from develop for the first stable release. Any new features not listed above should be kept in develop for the moment.

paulstapor commented 7 years ago

I think we got a confusion about who is doing the last task (although I wrote in my first post I would do it, but I didn't make up an issue of its own for that):

I did some work on that, which I consider to be basically complete (only the PesoOptions.m). But I don't want to push it, before I know that I don't mess up someone else's work. @dweindl : I pulled your last commit and as far as I see, I don't change things you did with one exception: I put an additional "%" in front of header lines, to make those things to appear more clearly in the Matlab editor. Please tell me, if this screws something up for doxygen (Or just remove the additional "%"s: it's not so many of them ;) ).

Do I have your okay to push my commit? @dweindl ? @BBallnus ?

dweindl commented 7 years ago

@paulstapor Doxygen stumbles over '%%' within the class body. Within a member function it is fine, but not outside. This header line will end up in the documentation for the next class member. That is why i changed them e.g. in 50672705d5643b70206fe20d9fb5b19d152d320a . Sounds okay for me to push, but I could give a more definite answer if you create a pull request.

paulstapor commented 7 years ago

Okay, I will remove those "%%". Then, I have basically nothing changed but rearranged the properties, to group them in a more intuitive way (and added a description for profile integration options).

dweindl commented 7 years ago

@paulstapor After changing the descriptions, please recreate the documentation (MatlabDocMaker.create) and check for warnings/errors in the log and shortly check the respective section in the pdf. Note: Comments within struct definitions unfortunately break mtoc++/doxygen.

paulstapor commented 7 years ago

I'll take care of the examples in the next 3 or 4 days (and will also take care of the pre-last point). Then I think, the list is finished... Maybe: The pre-pre-last point, we skip, right? Trying to ensure backward compatibility on something which has changed so much seems a bit pointless to me by now...