Export human readable format from standard dictionary

[vigneswaran-chandrasekaran]

Add human-readable format export from standard dictionary representation
Fixes #28
Fixes #41

[vigneswaran-chandrasekaran]

I guess, the PR is now ready for your kind review

[mstimberg]

This is really nice work. I'll make a few detailed comments in a minute, but these are very little things.
Unfortunately though, I also realized that the current structure with the Std_mdexpander class is not optimal. The problem is that if you want to build on it, you have to implement several functions that get additional arguments that are relevant to the Std_mdexpander but possibly not for other implementations. For example, create_md_string takes a github_md argument that most other exporters tailored to a specific use case will not be interested in. Similarly, it becomes hard to create a new "expander" that can take additional parameters that were not included in the current implementation. I think a good solution (and, hopefully, not too much work) would be to do the following:

1. Instead of passing a class as an argument to expander_class, we'll have an argument expander that takes an object.
2. The build function only contains the basic arguments that will be common to all implementations, i.e. direct_call, debug, expander, filename) – everything else goes as arguments into the Std_mdexpander object.

To make the second point clear:

A simple call with default parameters, e.g. device.build(debug=True) would be unchanged, for expander=None we create the Std_mdExpander object with default options.

A call with non-standard parameters would instead create its own Std_mdexpander object:

device.build(debug=True, github_md=True)

would be instead

custom_ expander = Std_mdexpander(github_md=True)
device.build(debug=True, expander=custom_expander)

I think this will also make the Std_mdexpander code a bit more readable, e.g. self.github_md will be set in __init__ instead of in create_md_string.

Does that make sense? I think this change is important because we commit to a general structure once this gets released, and I feel that the current approach will become problematic in the future.

PS: How about renaming Std_mdExpander into MdExpander for simplicity (consistent with MdExporter)?

[vigneswaran-chandrasekaran]

yes, that makes much sense, and I will do the necessary changes ☺️

[mstimberg]

As promised, here a few smallish things in addition to the slightly bigger structural change I mentioned in my earlier comment.

[vigneswaran-chandrasekaran]

Hi, thank you for the review!
I made the necessary changes and also, I noticed that when we expand initializers and connectors, we lost the information of their order, so I made a change to put them under the same section (something like: Initializing at start and Synaptic connection defined) such that the order is preserved. Would be happy to hear any better way to do this 👍
And that makes the PR ready for your kind review 😊

[mstimberg]

Looking good. I realized that unfortunately cannot use np.array2string because we want to keep the units... So I guess prepare_array has to do the little

np.set_printoptions(...)
result = str(arr)
np.set_printoptions(...)  # reset previous options

dance (Quantity.__str__ takes numpy's print options into account).

Another small thing that I just noticed: synaptic connections that use the generator syntax need to quote the syntax with back-ticks (`...`), otherwise it can potentially mess with the markdown syntax.

I noticed that when we expand initializers and connectors, we lost the information of their order, so I made a change to put them under the same section (something like: Initializing at start and Synaptic connection defined) such that the order is preserved. Would be happy to hear any better way to do this +1

I think this is the best option for now. I guess in the longer term we could do a bit of analysis and if the order of the operations does not matter (as is often the case), we could rather attach them to their objects which is a bit more readable.

As a general (again, minor) suggestion: I'd leave away the "defined", e.g. I'd just use "Initializations and synaptic connections". Also in other places, e.g. instead of "Synapses defined:", simply "Synapses:".

[vigneswaran-chandrasekaran]

Great! I'll do the changes.
And yeah, defined looks very redundant, and wonder why I wrote like that in the first place 😄

[vigneswaran-chandrasekaran]

Hi, a couple of things in the current version that I would like to highlight,

• When subgroups are created, no index details are mentioned

• To add custom expand class, the import statement for MdExpander has to be, from brian2tools.mdexport.expander import MdExpander

[mstimberg]

* When subgroups are created, no index details are mentioned

Good point! This is something that we have to fix in the base exporter, we need to store the source group and the start and stop for each subgroup (probably directly where they are used, since you can create subgroups on the fly). We don't really need their name on the other hand. I'll open an issue about this.

* To add custom expand class, the import statement for `MdExpander` has to be, `from brian2tools.mdexport.expander import MdExpander`

This isn't a problem, but we can always make it available with a shorter import if we want.

[mstimberg]

All is looking good to me. There's still a bit of polishing to do here and there, but I think it is ready to merge for now. Maybe a small thing to change (because it wlll also appear in the developer docs): since the expander_class argument is no longer a "class", maybe just rename it to expander?

[vigneswaran-chandrasekaran]

Cool! Can I make the subgroup changes in this PR itself?
And also, I will give a final full look to cover the possible enhancement changes and I'll request a review (also would be great to know if you happened to notice any straightforward changes)

[mstimberg]

Cool! Can I make the subgroup changes in this PR itself?

Sure! Sorry for the late reply...

[mstimberg]

This is all looking good to me, great work. Do you still want to add something in this PR? If not, I'll go ahead with the merge.

[vigneswaran-chandrasekaran]

Hi, thanks for your review :)
I had some couple of doubts and asked them in Gitter, if that is all good then I think I am done with my side

[mstimberg]

Sorry, I had accidentally logged out of gitter and did not see your question. I think it all looks good, the correct information for subgroups is shown and we can always tweak details of the formulation later.