Larch controller independent of Wx GUI + _data in _larch.symtable

[maurov]

@newville

In view of having Larch more independent from the Wx GUI, and have the possibility to build more Jupyter friendly examples (or applications) I would propose the following actions:

• create a new module larch.controller
• move all the non-Wx part from larch.wxxas.xas_controller.XASController to larch.controller.LarchController
• XASController will inherit from LarchController
• assign _larch.symtable as model for LarchController
• dedicate _larch.symtable._data as the main data container for all data groups created during a session
• add in LarchController all the logic for handling data exchange from the model to the view (= GUI)

Does this sounds good to you?

[newville]

@maurov

Hmm, that is an interesting idea. I'm not opposed, but I have some questions about what LarchController should do.... like, what are its methods?

Some of the design of larch.wxxas.xas_controller.XASController is sort of accidental. It does a few things:
a) handles merging of groups and also the mapping of "displayed group names" to "real group ids in the symbol table". That is probably something that any interface could use...
b) abstracts "Larch Session" so that it can either be a "plain larch interpreter" or the kind-of-fancy wx Larch Buffer.
c) handles get/set/save/restore of "configuration" settings for XAS Viewer. Many of these are not super-specific to a GUI, but give sort of default values to function call arguments. The application of those default arguments to the actual calls is not done by the controller (but maybe it should be?)
d) handles some of the user-interaction with plotting.

Not much of it is really wx specific. So, making a generic controller could be possible. It's even reasonable to say that it should do more that is currently scattered throughout XAS_Viewer. For example, "LarchController" could have methods for most of the XAFS processing methods so that one did LarchController.autobk which would use the config preferences and act on "displayed group names".

Is that what you would be looking for?

[maurov]

@newville

I do not have a clear design in mind yet, but I would keep a sort of MVC pattern.

As a first step, LarchController should have methods for:

• easily manage the exchange of groups and function call arguments between the symbol table and the rest of the world
• create command calls to the interpreter
• handle "Larch Session"

I would keep the Larch functions as they are.

Would also be fine for you if we store all groups and parameters in a dedicated group of the symbol table called _data, instead of having them at the root of the symbol table?

If you are not opposed to this idea, I could start coding something and propose it via a pull request.

[newville]

@maurov OK. For "MVC", I understand what "V" is, but am never sure of the difference between "M" and "C" unless "M" is "connection to a database server". For sure, "real functionality" and "GUI code" should be separate, well separate-ish. Or, some things a GUI "does" can be made into functions that can be called in other ways.

But in analogy with a database server, "Larch Session (data + functions)" could be "M", and then "C" is pretty close to trivial.

Would also be fine for you if we store all groups and parameters in a dedicated group of the symbol table called _data, instead of having them at the root of the symbol table?

Maybe.... I'm not sure what that solves.

[maurov]

@newville I wanted to implement a simple feature request from a user (related to the Wx GUI) and I realize that I still do not fully understand how the controller manages things between the Larch interpreter and the Wx GUI.

Some of the design of larch.wxxas.xas_controller.XASController is sort of accidental. It does a few things: a) handles merging of groups and also the mapping of "displayed group names" to "real group ids in the symbol table". That is probably something that any interface could use... b) abstracts "Larch Session" so that it can either be a "plain larch interpreter" or the kind-of-fancy wx Larch Buffer. c) handles get/set/save/restore of "configuration" settings for XAS Viewer. Many of these are not super-specific to a GUI, but give sort of default values to function call arguments. The application of those default arguments to the actual calls is not done by the controller (but maybe it should be?) d) handles some of the user-interaction with plotting.

Yes, to me this is exactly the role of a controller object and I would like to better understand the details. Please, could you comment/reply to the following points, related mainly to "a" and "b" points of your previous reply?

• controller = larch.wxxas.xas_controller.XASController()
  • controller.symtable is the model (that is, an in memory container storing all data, metadata, configs, etc. we use a Group object, but could be anything - dictionary, database, etc.)
  • generally, which attribute uniquely identifies a group? __name__, groupname or filename? I think the symtable uses groupname, but sometimes other functions use filename too. If it is groupname, then a g = Group() should instantiate it (it is not the case, only g.__name__ exists)
  • [most important] I propose adding controller.add_group(group) method to programmatically add a new data group to the model. It is not clear to me how to do it. I think we could move XASFrame.install_group method to the controller. In fact, that method is used to add groups when opening a file, but to me it would make more sense to have it in the controller (by the way, why install_group requires both groupname and filename to identify a data group?). There is also controller.copy_group(filename) but it is not clear to me why this method uses filename instead of groupname
  • I do not understand the role of controller.fie_groups in controller.get_group(groupname) to get a given group from the model
  • I propose adding controller.get_data_groups() method to get the list of the unique identifiers of the data groups in the model. If my understanding is correct, this should simply be return list(controller.symtable._xasgroups.values())?
  • controller.larch is the main Larch interpreter? why its type is larch.wxlib.larchframe.LarchWxShell? I was expecting larch.interpreter.Interpreter. Well, probably in the future we will not need the interpreter anymore...

[newville]

@maurov Well, I should restate my view that MVC is one approach to UI. The separation between the 3 objects is often not completely clean. For sure, it is good to keep "real code" out of a GUI. Having the GUI react to input and generate (without loss of generality) some sort of dictionary or another simple mapping of "GUI Component" to "Value" is good. That is, "View" is usually clear.

Some of the design of larch.wxxas.xas_controller.XASController is sort of accidental. It does a few things: a) handles merging of groups and also the mapping of "displayed group names" to "real group ids in the symbol table". That is probably something that any interface could use... b) abstracts "Larch Session" so that it can either be a "plain larch interpreter" or the kind-of-fancy wx Larch Buffer. c) handles get/set/save/restore of "configuration" settings for XAS Viewer. Many of these are not super-specific to a GUI, but give sort of default values to function call arguments. The application of those default arguments to the actual calls is not done by the controller (but maybe it should be?) d) handles some of the user-interaction with plotting.

Yes, to me this is exactly the role of a controller object

Well, XASController mostly is there to manage the mapping between the displayed "File Names" and "Group Name", which is the real name of the Python symbol in the symbol table. And it holds the Larch Session.

But Python is not strictly OO, so the GUI code might be able to access the Larch interpreter or its symbol table directly. Does doing that violate "MVC"? Maybe. If so, strict MVC becomes an anti-goal.

controller.symtable is the model (that is, an in memory container storing all data, metadata, configs, etc. we use a Group object, but could be anything - dictionary, database, etc.)

I would probably say that the Larch session is the Model. But, all the data and code are in the symbol table.

generally, which attribute uniquely identifies a group? name, groupname or filename? I think the symtable uses groupname, but sometimes other functions use filename too. If it is groupname, then a g = Group() should instantiate it (it is not the case, only g.name exists)

groupname is the name of the Group object in the Larch symbol table. filename is what is displayed as the Name in the GUI -- likely derived from the name of the original data file. The naming conventions for groupname are strict enough (stricter than "Python symbol name", in fact matching [a-z_][a-z0-9_]*) that the flexibility of filename is important.
Each Group can have a __name__ attribute. That's sort of a programming convenience.

The XASController.file_groups == XASController.larch.symtable._xasgroups dict keeps the mapping between filename and groupname.

[most important] I propose adding controller.add_group(group) method to programmatically add a new data group to the model. It is not clear to me how to do it. I think we could move XASFrame.install_group method to the controller. In fact, that method is used to add groups when opening a file, but to me it would make more sense to have it in the controller (by the way, why install_group requires both groupname and filename to identify a data group?). There is also controller.copy_group(filename) but it is not clear to me why this method uses filename instead of groupname

Good point, I agree that "install_group" could be moved to the controller. I'll do that. I'm still in the process of re-doing data importing and working on the Energy Calibration dialog, so this is a fine time to do that.

I do not understand the role of controller.fie_groups in controller.get_group(groupname) to get a given group from the model

file_groups is the dictionary with keys of "File / Display Name" and values of groupname.

I propose adding controller.get_data_groups() method to get the list of the unique identifiers of the data groups in the model. If my understanding is correct, this should simply be return list(controller.symtable._xasgroups.values())?

Yes, that is just controller.file_groups.values(). Does it need a separate method?

controller.larch is the main Larch interpreter? why its type is larch.wxlib.larchframe.LarchWxShell? I was expecting larch.interpreter.Interpreter. Well, probably in the future we will not need the interpreter anymore...

The "LarchWxShell" is a subclass of Larch Interpreter that is the viewable / interactable "Larch Buffer". That is some very old GUI interface and probably needs some attention. Having that use the Jupyter tooling would be interesting and worth investigating.

I think that XAS Viewer / Larix will keep some interpreter as the base Model - the model of "GUI generates code that can be run separately or turned into a script" is a feature we want to keep.

It could happen that "Larch interpreter" gets replaced by "asteval Interpreter" (especially if Jupyter tooling is used), but that would actually be some work, as the Larch symbol table is sort of hierarchical whereas the asteval one is a flat dict. I am totally willing to say "flat is better", but that would mean touching a lot of Larch code. But, that might be "Larch 1.0" ;).

[maurov]

@newville thank you very much indeed for the detailed reply. I think I better understand now your design choices. I do not care to strictly follow the MCV paradigm, it was for me just following a scheme to understand the design. No problem with leaving it as it is now.

It would be great if you could move install_group() to controller.add_group(), - or controller.install_group() if you prefer this naming. Thanks.

I fully agree on the other points.

[maurov]

@newville thanks for implementing controller.install_group. Unfortunately, I still have problems using it for adding a new group not already existing in the symbol table. What about having controller.add_group(group) which adds a new group? What I would expect from this method is this:

g = Group(groupname="my_new_group_id_abc123", filename="My fancy new group this and that", energy=xarray, mu=yarray, i0=numpy_ones_like(xarray), datatype='xas')
controller.add_group(g)

by the way, is i0 attribute always required to correctly identify a "xas" datatype?

[newville]

@maurov

What about having controller.add_group(group) which adds a new group? What I would expect from this method is this:

I will freely admit that the xasgui.XASFrame and xas_controller.XASController have a lot of overlap and could be better separated. But also, MVC is not the goal. It might be means to that goal, but it is not the goal.

xasgui.install_group does accept either a Group or the name of a group in the Larch symbol table. xas_controller.install_group expects the name of a Group in the Larch symbol table, and doesn't check for whether the first argument is a group or the name of a group.
FWIW, currently you could do something like

controller.larch.eval("""my_new_group = group(energy=xarray, ....)""")
g = controller.symtable.get_group["my_new_group"])
g.energy = g.xdat = xarray
g.mu = g.ydat= yarray
controller.install_group("my_new_group", "My File Name")

But we could more completely make "XASController.install_group" and "XASFrame.install_group" be the same -- it was sort of ambiguous how to handle what the dialogs do to create new groups, and especially whether to process and plot the data on installation of a group.

Or we could add a separate "add_group()" method that requires the first argument to be a Group that is not already in the Larch symbol table. The downside there is that it probably will not be well documented where that group came from.

by the way, is i0 attribute always required to correctly identify a "xas" datatype?

No, or it should not be. If the datatype is xas, i0 is looked for.

[maurov]

@newville

a group. FWIW, currently you could do something like

controller.larch.eval("""my_new_group = group(energy=xarray, ....)""")
g = controller.symtable.get_group["my_new_group"])
g.energy = g.xdat = xarray
g.mu = g.ydat= yarray
controller.install_group("my_new_group", "My File Name")

Unfortunately this does not work for me g = controller.symtable.get_group("my_new_group") gives a NameError.

Or we could add a separate "add_group()" method that requires the first argument to be a Group that is not already in the Larch symbol table. The downside there is that it probably will not be well documented where that group came from.

It think it would be very beneficial to have controller.add_group(group) as unified way to add a new (not existing) group in the symbol table. This method should also check if the group already exists. To document from where the group came from, add_group could accept a journal="group created from somewhere" argument. What do you think?

by the way, is i0 attribute always required to correctly identify a "xas" datatype?

No, or it should not be. If the datatype is xas, i0 is looked for.

Ok, thus this means that when datatype="xas" is given one has to create group.i0, otherwise things may go wrong.

[newville]

@maurov

Unfortunately this does not work for me g = controller.symtable.get_group("my_new_group") gives a NameError.

Hm, that should work I think.

It think it would be very beneficial to have controller.add_group(group) as unified way to add a new (not existing) group in the symbol table. This method should also check if the group already exists. To document from where the group came from, add_group could accept a journal="group created from somewhere" argument. What do you think?

How should add_group differ from install_group? Currently (but recently, and maybe subject to change with all of this) the current dialogs mostly expect that they are "installing" named groups in the symbol table with the meaning of "install" to be "expose to the XAS Viewer File List".

Ok, thus this means that when datatype="xas" is given one has to create group.i0, otherwise things may go wrong.

I don't think so, or at least that is not the intention. I added 'i0' recently mostly so it could be consistently / easily plotted with "mu". None of the downstream processing will really use this.

[maurov]

@newville

Unfortunately this does not work for me g = controller.symtable.get_group("my_new_group") gives a NameError.

Hm, that should work I think.

Yes, I also do not understand why is not working. Here a specific example of what I am trying to do (not working yet!):

master...maurov:xraylarch:add_export_group_from_lcf

It think it would be very beneficial to have controller.add_group(group) as unified way to add a new (not existing) group in the symbol table. This method should also check if the group already exists. To document from where the group came from, add_group could accept a journal="group created from somewhere" argument. What do you think?

How should add_group differ from install_group? Currently (but recently, and maybe subject to change with all of this) the current dialogs mostly expect that they are "installing" named groups in the symbol table with the meaning of "install" to be "expose to the XAS Viewer File List".

Well, to me controller.add_group(group, journal="description") and controller.install_group(group, journal="description") should do the same thing, that is:

• add (if not existing) or update (if existing) the given group to the symbol table (the model)
• insert an entry in the journal
• make XAS Viewer File List (the viewer) aware there is a new/updated group in the model

I am sorry to come back to the MVC story, but I am not very familiar with GUI development and I find easier reasoning in a MVC way.

Ok, thus this means that when datatype="xas" is given one has to create group.i0, otherwise things may go wrong.

I don't think so, or at least that is not the intention. I added 'i0' recently mostly so it could be consistently / easily plotted with "mu". None of the downstream processing will really use this.

I think there are some places in the code where "datatype = 'xas' requires i0 attribute". It is good to know this is not the intention and I will fix it if I will come across it.

[newville]

Yes, I also do not understand why is not working. Here a specific example of what I am trying to do (not working yet!):

master...maurov:xraylarch:add_export_group_from_lcf

What is the error message ? Like, what line is failing? One thing that can be very useful for debugging here is to look at that command that got executed in the Larch buffer. There, you can check for errors running that command and really see what the outputs are (including, say the group name).

Well, to me controller.add_group(group, journal="description") and controller.install_group(group, journal="description") should do the same thing, that is:

If they do the same thing, why do we need both methods?

I think there are some places in the code where "datatype = 'xas' requires i0 attribute".

Hm, where is that? I would say it is better to have 'i0', but should not be required.

[maurov]

What is the error message ? Like, what line is failing? One thing that can be very useful for debugging here is to look at that command that got executed in the Larch buffer. There, you can check for errors running that command and really see what the outputs are (including, say the group name).

Looking at the Larch buffer permitted understanding what was the problem, thanks for the help!

Well, to me controller.add_group(group, journal="description") and controller.install_group(group, journal="description") should do the same thing, that is:

If they do the same thing, why do we need both methods?

Because to add a new group one needs to perform more than simply a call to controller.install_group. Here what I have to do now for example:

        controller = self.parent.controller
        label = f"lcf_fit_{nfit}"
        groupname = new_group = f"{dgroup.groupname}_{label}"
        filename = f"{dgroup.filename}_{label}"
        cmdstr = f"""{new_group} = group(name="{groupname}", groupname="{groupname}", filename="{filename}")"""
        controller.larch.eval(cmdstr)
        g = controller.symtable.get_group(new_group)
        g.energy = g.xdat = xarr
        g.mu = g.ydat = g.norm = yfit
        g.i0 = i0
        g.datatype = 'xas'
        controller.install_group(groupname, filename, source="exported from Linear Combo / Fit Results")

to me it would be much easier to handle all this behind the scene and simply doing

g = Group(...)
controller.install_group(g, source="created from something")

I think there are some places in the code where "datatype = 'xas' requires i0 attribute".

Hm, where is that? I would say it is better to have 'i0', but should not be required.

I may be probably wrong. If I find it, I will open a separate issue.