complexvariables / conformalmapping

Conformal mapping toolkit for MATLAB
Other
11 stars 9 forks source link

Implementing preferences #13

Closed tobydriscoll closed 10 years ago

tobydriscoll commented 10 years ago

My proposal is to create two functions, cmt.set(module,prop1,val1,...) and cmt.get(module,prop1).

Then there is a three layer process in each module:

  1. Set defaults for your module.
  2. Apply anything that was given in a cmt.set.
  3. Apply any passed-in parameter/value pairs.
tobydriscoll commented 10 years ago

Upon review, this can be refined.

Have each module (class) define its own static set and get methods to handle (adjustable) defaults. Then internally they can use the cmt static methods to avoid code duplication.

ehkropf commented 10 years ago

I'm pretty sure I get the three layer part, but I don't think I understand how you might get there with set and get methods. Pretend we have a class itWorks which implements this. Could you give the set of commands you have in mind which shows this in action?

Some preferences are already handled by subclassing the optset object. See how szego and szmap use szset for example. (I should probably put an example of use of szset somewhere.) So I'd like to be clear that you mean preferences for things like plot behaviour?

ehkropf commented 10 years ago

Ok, I think I have something via some odd but documented handle class behaviour. Check out the features/preferences branch on my personal fork, and take a look at prefsdemo.m. It gives us the get(cmtpref, 'pref1') and set(cmtpref, 'pref2', 12) syntax, along with persistent, global-like preference values.

ehkropf commented 10 years ago

Whoops, I guess I should add to be clear that the prefsdemo stuff is what I have in mind for things like plotting preferences (which now I think can meet your 3-tier specification). For things like szmap, I think I prefer having something like szset, which seems more in line with "the matlab way" (I'm thinking of things like odeset). On reflection, imitating matlab's behaviour possibly isn't the best way to go; it being the exemplar of software design that it is.

tobydriscoll commented 10 years ago

I have a working model for the SC diskmap. See the branch feature/sc-toolbox.

ehkropf commented 10 years ago

Very clever, using the app data facility; I'd completely forgotten about it. I think I see what you have in mind now. I'll see about getting rid of the use of optset and child classes, as well as adapting the plots to use the app data mechanism.

ehkropf commented 10 years ago

What if we continue to use the children of the optset class (or something like it) and store the instances of these classes in app data instead of bare preference structures?

ehkropf commented 10 years ago

Ok, modification of your idea. Scrap the static get/set functions in cmt. Create a new class cmtbase or something like that. This will be the parent class of all base classes, e.g., conformalmap, closedcurve, and region will all be subclasses of cmtbase. In this base class, we define non-static get/set methods, which can auto detect from which class they are being called by simple inheritance. Further, unless a child class sets an optsClass property with the name of an optset subclass, the get/set methods basically do nothing. If however this property is set, then the preferences are stored and retrieved via the app data facility in line with what you had planned.

For example, check out the "issues/#13-preferences" branch in my fork, and run the following code.

get(szego)
set(szego, 'numCollPts', 1024)
get(szego)

(I haven't changed szego here yet to fully implement this. It's currently just a test of the new get/set scheme, so it's a pretty fragile example.)

This scheme gives us advantages such as

tobydriscoll commented 10 years ago

Slick.

One thing I don't understand is, why can't a class just not be derived from cmtbase if it has no options to be set? The optsClass bit seems unnecessarily complicated, unless I'm missing something. I don't mind getting errors for commands that don't exist.

ehkropf commented 10 years ago

So I was obviously over exuberant when proclaiming a universal base class, but I did have in mind its use for plot defaults as well, so it would make it fairly widely subclassed. I got the idea from Qt, which has a QObject base class which you don't need to subclass to use Qt, but it adds a lot of magic if you do.

The optsClass property is so far the best thing I can come up with to pass back up the inheritance chain, so to speak, which class to use for preferences. Note that you don't actually have to define an optsClass property to use the get/set mechanism. You can also use it to store a plain structure if you don't define this. Though it just hit me there's no mechanism for defaults without the optsClass. So that's a drawback.

Note also this scheme requires the subclass to be able to be instantiated with no arguments, i.e., it must conform to the no argument case discussed in the matlab OO docs.

On second thought, if a class doesn't want to use the optset mechanism, then the class could simply set its own defaults in its constructor. Thus a class in this category would just call something like

set(classname, 'pref1', val1, ...)

or

set(object, 'pref1', val1, ...)

as appropriate, which would simply store the data in the basic pref.module.field structure format.

ehkropf commented 10 years ago

Unintended side effect: If any optset based preferences are stored in app data, a call to clear classes will fail on complaint about instances of classes still existing. Current solution: function cmt.clearPrefs calls

rmappdata(0, 'cmt_prefs')
tobydriscoll commented 10 years ago

The clear classes issue only affects developers, really.

One more question: Why does the optsClass property need to be the name of a class? Why can't the szego class have a defaultOptions_ property that is a szset object?

ehkropf commented 10 years ago

Good point, I imagined there was a circularity problem where there wasn't.

Possibly even better, still using szego for the sake of example, how about during construction it does something like

function S = szego(...)
    ...
    prefs = get(S, szset);
    ...
end

where the szset instantiation provides the defaults and the appropriate optset child class? Again if we don't want to use the optset mechanism for a class, then a call like

function C = sample(...)
    ...
    % some defaults
    prefs = struct('pref1', val1, 'pref2', val2, ...);
    prefs = get(C, prefs);
    ...
end

would also provide defaults. Thus there is no extra property needed in the subclass in either case.

tobydriscoll commented 10 years ago

I like it. Do we even need the second alternative?

ehkropf commented 10 years ago

Probably not, but the code to handle it is already 99% there as an unintended consequence of doing the first. Should we preclude the possibility of the second?

tobydriscoll commented 10 years ago

For future readers and contributors, it's less confusing if there is just one way of doing things to understand. Unless there's a use case for the non-derived scenario, let's leave it out.


Tobin Driscoll Professor and Director of Graduate Studies Department of Mathematical Sciences University of Delaware, Newark, DE 19716 Twitter: @tobydriscoll https://twitter.com/intent/user?screen_name=tobydriscoll Gplus: +TobyDriscoll http://google.com/+TobyDriscoll

On Mon, Jul 28, 2014 at 4:32 PM, Everett Kropf notifications@github.com wrote:

Probably not, but the code to handle it is already 99% there as an unintended consequence of doing the first. Should we preclude the possibility of the second?

— Reply to this email directly or view it on GitHub https://github.com/tobydriscoll/conformalmapping/issues/13#issuecomment-50395611 .

ehkropf commented 10 years ago

I won't disagree with that. It shall be so.

ehkropf commented 10 years ago

Change is parked in a pull request, please take a look. If you don't spot anything, it's ready to merge. (Just want a second pair of eyes on it before it goes into master.)

ehkropf commented 10 years ago

I needed the change for something else, so I merged it. It passed all the testing I threw at it, along with the spec files. If it has problems that's what patches are for.

Is this issue sufficiently addressed to close?