BrainDynamicsUSYD / nftsim

C++ library for simulation of multiscale neural field dynamics
Apache License 2.0
29 stars 24 forks source link

Parser refactoring #22

Open pausz opened 9 years ago

pausz commented 9 years ago

From @RomeshA

Related to #19

If we're changing the parser, I wonder if it would be better to separate the dendrites from the populations - there are as many of them as there are couples, of course. They are presently contained within the population blocks because of the way that the config file is currently read, but I think this makes the config file more confusing to read.

felixfung commented 9 years ago

Hi Romesh,

As has happened on occasions, we disagree on some design issues.

The interface should conform to the code, the code does not conform to the interface.

It should be easy to write matlab or python interface (even gui). These would be interfaces to an interface, the config file. Remember one of the strengths of NeuroField is the config file and the option for direct use, similar to outputting.

Manual config file is an important option, with many collaborators sharing and reading it directly. Therefore, the integrity and presentation of it must be kept. Imagine looking directly at an eirs config file, without arbitrary white space to align columns, config files are very unreadable, and new line delimiter definitely makes it unprintable.

C++ code should be separate and independent from other scripts. It is unreasonable to refactor big chunks of code to accommodate an interface to an interface. In this particular case, I don't even see how strict/loose parameter ordering has got to do with matlab parsing. If you want a "general" matlab or python interface, the best option is to require the developer to supply the parameter option in a specific format, which the matlab/python interface reads off. This way, code integrity/flexibility will not be compromised, while strongly encouraging up-to-date documentation.

Concerning what a logical block in a config file is: this is exactly the problem. With the object hierarchy and inheritance structure, there are always sub-objects within objects, and derived classes calling base classes. The reader/writer of the config file should not worry about what are the objects. The coder certain CANNOT foresee or rule out re-using existing code, or the current code in development being re-used in the future. The current "stupid" way does the job quite well. I challenge you to find another way without exploding future development time and API friendliness, as well as minimizing a user's chance of stupid mistakes (which may be quite subtle and take a long time to find out!).

Labeling of populations and connections may be done, but there is no need to change the underlying vector (Array) structure. An auxiliary (vector based) object in Solver that reads letters from the connection matrix:

Connection matrix: To: e i r s n From e: 0 0 0 0 0 From i: 0 0 0 0 0 ...

so that populations 1,2,3,4,5 has IDs e i r s n. Correctly calling the init and step functions for each object is simple.

Regarding dendrites, perhaps some others would disagree with you concerning confusion. In TMS modeling the current presentation actually helped some collaborators in clarifying certain modeling issues. From a coding standpoint, Dendrites is an object in QResponse which is an object in Population. To have the interface otherwise would involve breaking encapsulation and creating auxiliary objects.

If we're changing the parser, I wonder if it would be better to

separate the dendrites from the populations - there are as many of them as there are couples, of course. They are presently contained within the population blocks because of the way that the config file is currently read, but I think this makes the config file more confusing to read.

There should be a way to name populations and give them a single letter subscript, which would then enable us to refer to populations with labels like 'e','r','s', and to connections like 'ee','ei'... rather than population 1, couple 3. This would also make it easier to compare similar components when the model is changing and thus the indexes might change.

I could imagine a solution using something like Python dictionaries to have

key:value pairs, where the key would be 'ee', 'ei' for connections and 'e', 'i'. I think numeric indexing is more general, so labels are mainly a human readable decoration. Maybe we can generate a mapping between the two? Apparently the equivalent dictionary structure in C++ is called std::map http://en.cppreference.com/w/cpp/container/map. However, I've never used it.

The order in which parameters are entered matters, and the order is

determined by the ordering of the statements in the init() function in the cpp files, which means that there is no way to know what the correct order is without inspecting the file. Problems: This makes it impossible to implement the general parser without somehow modifying it to store the correct order. The solution to this must be to make the config files order-invariant. One possibility would be to rewind the file pointer after each parameter is read, back to the original position from which the init function was called. We would need to check carefully what conditions this might impose on having duplicate or similar parameter names. In this case, we would still need to somehow return from the init function with the file pointer at the end of the block (in preparation for the next block) - this could be difficult. Potential Solution: One option is to make the config file whitespace-dependent (and then use newlines to separate logical blocks)? Questions: What's a logical block in the config file? An object with its parameters?

A general Matlab interface needs to be able to specify options for

any NeuroField object. However, the parameters of the object are different for each of the classes, as well as for derived classes. Therefore, the Matlab interface needs to be able to generate strings for arbitrary parameters, which should be fine because the parameters follow a standard syntax (with colons and dashes).

On Fri, Aug 14, 2015 at 12:24 AM, Paula notifications@github.com wrote:

From @RomeshA https://github.com/RomeshA

Related to #19 https://github.com/BrainDynamicsUSYD/neurofield/issues/19

If we're changing the parser, I wonder if it would be better to separate the dendrites from the populations - there are as many of them as there are couples, of course. They are presently contained within the population blocks because of the way that the config file is currently read, but I think this makes the config file more confusing to read.

— Reply to this email directly or view it on GitHub https://github.com/BrainDynamicsUSYD/neurofield/issues/22.

Felix Fung, PhD Postdoctoral Associate ------------------------------------------- Department of Ophthalmology Downstate Medical Center State University of New York 450 Clarkson Avenue, MSC 58 Brooklyn, NY 11203, USA --------------------------------------------- Office: +1 (718) 270-2100 Mobile: +1 (646) 683-8709 _E-mail: _fylixeoi@gmail.com fylixeoi@gmail.com _E-mail: _ffung@physics.usyd.edu.au ffung@physics.usyd.edu.au