search

gkaguirrelab / temporalFittingEngine

A core computational engine, and associated models, for non-linear fitting of parametric models to time-series data
0 stars 0 forks source link

Coding style question #9

Closed DavidBrainard closed 8 years ago

DavidBrainard commented 8 years ago

Denis Pelli always says: "Respect the user's control of the command window' and I tend to agree. Thus I think that functions should do minimal diagnostic printing into the command window. Or more generally that such diagnostics should be controlled by a 'verbose' key/value pair, whose default can be true when we're developing and set to false later. Possibly this key/value pair should be accepted by the top level calls in tfe and the model objects and passed along so it can be turned on and off easily.

I am thinking about this because I see instances of diagnostic printout without any conditional.

gkaguirre commented 8 years ago

Sounds good. So the tfe model object would take the verbose key/value pair, and this behavior would be respected by the subclass models.

The isPacket routine issues warnings when the packet is not valid. I would think that these warnings would be issued regardless of the verbose setting, and that the calling routine could choose to turn warnings off if it wished. Sound about right?

spitschan commented 8 years ago

I thought about this, since I like to look fmincon's output and it's quite useful for debugging. Perhaps this could in fact be done as a preference in MATLAB?

DavidBrainard commented 8 years ago

So one option is to pass key-value pairs around, and another is to use a local hook to set a preference for the project. Or perhaps even some hybrid of the two. Avoiding preferences is good, I think, for as long as you can get away with it. But maybe this is a good application.

DavidBrainard commented 8 years ago

I implemented a verbosity key/value pair in parent class tfe. This allows you to access string obj.verbosity (current values 'none' and 'high', but if you put in a case for some other value (e.g. 'medium') in a method you'd only need to change the comment in tfe that lists the allowable variables.

I tested that this works for tfeQCM and put in the code for the constructors of other models. See pull request.

Closing this issue, since if this doesn't work we'll just consider it a bug and create a new isseu.