Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arbor internal syntax parser is coded in bluepyopt #494

Open
anilbey opened this issue Jun 19, 2024 · 13 comments
Open

Arbor internal syntax parser is coded in bluepyopt #494

anilbey opened this issue Jun 19, 2024 · 13 comments

Comments

@anilbey
Copy link
Contributor

anilbey commented Jun 19, 2024

This module for parsing Arbor syntax is part of BluePyOpt. Changes in newer versions of Arbor cannot be reflected in this module because it is integrated within BluePyOpt. To ensure BluePyOpt remains up-to-date with the latest Arbor versions, this module should be imported directly from Arbor.

class ArbIExprValueEliminator(ast.NodeTransformer):

@thorstenhater
Copy link

Hi @anilbey,

I found this by accident and as the resident Arbor person I thought I'd offer a hand. @lukasgd has made the BluePyOpt
to Arbor interface happen, so maybe he has thoughts, too. The problem is, as time passes, APIs and formats will evolve
and in this particular case, both did change. The ACC file version is now '0.9.0', matching the Arbor version and we tweaked
the way inhomogeneous parameters are serialized and represented at the API layer.

The API, including the parsers, are public. You'll find them by looking for load_component. That said, using this method
on the old files will result in an Exception due to version mismatch. If you want help updating this, just ping us.

@anilbey
Copy link
Contributor Author

anilbey commented Jun 20, 2024

Hi @thorstenhater thanks for offering your help. I will not be around (I am leaving soon) but the maintainers of BPO can help.
cc @AurelienJaquier @ilkilic @darshanmandge

@lukasgd
Copy link
Contributor

lukasgd commented Jun 20, 2024

Hi @anilbey and @thorstenhater,

Thanks for bringing this up in an issue. I think there's a bit a misunderstanding with the purpose of the acc_iexpr module. It's not an Arbor syntax parser, but it's actually generating ACC expressions from BluePyOpt's internal representation of inhomogeneous parameters. The main interface is the method generate_acc_scale_iexpr in that module.

This functionality is necessary so that BluePyOpt can export models that can be run with Arbor and, therefore, also support Arbor as an optimization backend. When writing the ACC exporting infrastructure, I tried to stay as consistent as possible with the HOC/Neuron code, so that BluePyOpt could be used as a frontend to these different simulator backends. As such, it needs to be able to export model state to drive optimizations and store results.

Having said this, I see your concern of having the exported format getting out of date with what Arbor expects. At the moment the tox tests on my system (Python 3.10) are still succeeding as I installed Arbor as a dependency through pip (v0.9.0), but the current main branch should introduce an exception as Thorsten says above.

I believe this should already be visible in CI currently, since e.g. simplecell_arbor.ipynb is executed as part of it. The fix may then be done in the templates at bluepyopt/ephys/templates/acc. To regenerate the test data, you can use the generate_acc.py scripts in the examples directory (l5pc, simplecell and expsyn).

Back when Arbor support was added, we did not add L5PC_arbor.py to CI (instead focused on a consistency check between Arbor and Neuron with @wvangeit), so I it may be true that iexprs are not parsed by Arbor in any test currently. If you'd like to do that, e.g. for the expression at https://github.com/BlueBrain/BluePyOpt/blob/master/bluepyopt/tests/test_ephys/test_parameterscalers.py#L106-L109, you could use something inspired by this code

import pathlib
import tempfile
import arbor

# iexpr from bluepyopt/tests/test_ephys/test_parameterscalers.py
iexpr = '(sub (scalar 0.62109375) ' \
        '(mul (log (pi) ) ' \
        '(exp (div (distance (region "soma")) ' \
        '(scalar 0.421875) ) ) ) )'

# modified decor as in bluepyopt/tests/test_ephys/testdata/acc/simplecell/simple_cell_decor.acc
simple_cell_decor_with_iexpr = \
    '(arbor-component\n' \
    '  (meta-data (version "0.1-dev"))\n' \
    '  (decor\n' \
    '    (paint (region "soma") (membrane-capacitance 0.01))\n' \
    '    (paint (region "soma") (scaled-mechanism (density (mechanism "default::hh" ' \
   f'("gnabar" 0.10299326453483033) ("gkbar" 0.027124836082684685))) ("gkbar" {iexpr})))))'

with tempfile.TemporaryDirectory() as test_dir:
        decor_filename = pathlib.Path(test_dir).joinpath("decor.acc")
        with open(decor_filename, "w") as f:
            f.write(simple_cell_decor_with_iexpr)
        test_decor = arbor.load_component(decor_filename).component
        print(f'decor {decor_filename} with\n'
              f'  defaults: {test_decor.defaults()}\n'
              f'  paintings: {test_decor.paintings()}\n'
              f'  placements: {test_decor.placements()}')

which currently still works on my system.

Probably more important would be to add additional arbor.load_component statements like in

ref_labels = arbor.load_component(ref_dir / label_dict_acc).component
to that file (this one should also trigger a version mismatch exception btw). If you search e.g. for l5pc in that file, you'll see that the test checks for string equality of the label_dict and decor ACC files. It think it wouldn't harm to add more checks below, such as an arbor.load_component on these files. This would then guarantee that all three exported models - simplecell, l5pc and expsyn - are parsable in Arbor, so a faulty iexpr in l5pc would be caught in CI.

@thorstenhater
Copy link

We also add a regular run of a simple BPO script to Arbor's CI to ensure we do not break you code.

@AurelienJaquier
Copy link
Collaborator

Hi @thorstenhater @lukasgd , it looks like since the new 0.10 arbor release, our tests here on BluePyOpt do not pass anymore. Is this related to this issue or is it something new?
I tried using generate_acc.py in the l5pc example with an up-to-date arbor version, but it does not bring any changes with respect to the files we are already using in the tests. Am I misunderstanding something?
Could you guys make BluePyOpt and its tests compatible with latest arbor please?
Here is a failing test for reference: https://github.com/BlueBrain/BluePyOpt/actions/runs/10336590694/job/28612509119

@thorstenhater
Copy link

Hi @AurelienJaquier,

it seems your tests are using a fixed .acc file, however, I am not familiar with the details.
The format was changed due to an update to the handling of inhomogeneous parameters,
thus the format metadata was changed to 0.9.0-dev (the Arbor version of the update).

Best,
T

@wvangeit
Copy link
Contributor

Is there maybe an automatic converter from the old format to the newer?

@thorstenhater
Copy link

No, currently not. A

eous parameters are serialized and represented at the API layer.

The API, including the parsers, are public. You'll find them by looking for load_component. That said, using this method on the old files will result in an Exception due to version mismatch. If you want help updating this, just ping us.

Btw, this is precisely what I warned about on 20th of June.

@AurelienJaquier
Copy link
Collaborator

I opened a PR to make BluePyOpt compatible with latest arbor and make the tests pass: #498
Is it ok if I ask to to review it in order to have a look from the arbor team @thorstenhater ?

@lukasgd
Copy link
Contributor

lukasgd commented Aug 21, 2024

Hi @AurelienJaquier, thanks for your work!

I'm just having a brief look (would have more time in a month). I think you've correctly identified the acc/*_template.jinja2 files that need to be modified. They're used by all model exporters (also in examples), which is why the generate_acc.py failed for you before. For the specific changes, including the (scalar 1.0) multiplier for default and painted parameters, maybe Thorsten can comment.

There are some other changes to morphology (only for SWC) and protocols (stimulus envelope, units) that I'm not sure if they are required for compatibility. Did something not work there before?

Also, to return to the earlier point about checking compatibility in tests - have you considered adding arbor.load_component to tests as suggested at the end of this comment?

@AurelienJaquier
Copy link
Collaborator

Hi @lukasgd thank you for checking my PR.
Indeed, I had to change the morphology loading and adding units because without that the tests were not passing:

  • changes to morphology:
    for some reason, some morphology format return a loaded_moprhology instead of a morphology, which has not the same attributes, so I had to make sure they are all of type morphology to be able to use them consistently
  • changes to units:
    it looks like a lot of places in arbor now do not accept numbers anymore, but require a value associated with a arbor unit, so I had to update those.

About the load_component tests, I have added the one depending on iexpr, as you described above. Could you have a look and tell me if it looks ok to you? I would like to be sure I am doing it correctly before doing the other ones (singlecell, l5pc and expsyn)

@thorstenhater
Copy link

thorstenhater commented Aug 21, 2024

Hi @AurelienJaquier,

we made some changes to our I/O support. loaded_morphology (naming is hard) is essentially this

{
  morphology  # the actual morphology
  segment_tree # raw segment information
  label_dict # table of labelled locations on the morphology, if any      
  metadata # loader specific information
}

Thus, all loaders share one interface and do more for the user.

Units is also a new feature in v0.10.0, requiring the explicit use of a unit at the interface level.

Since BluePyOpt+Arbor already depends on Arbor, is using something like this an option?

import arbor as A

cell = A.cable_cell(morphology, decor, labels)
A.write_component(cell)

This will use our internal serialization to .acc making these templates redundant.

@lukasgd
Copy link
Contributor

lukasgd commented Aug 22, 2024

W.r.t. using Arbor for serialization vs. templates, see #498 (comment).

For the iexpr loading test @AurelienJaquier, I think you could already stop at the line test_decor = arbor.load_component(decor_filename).component. Invoking arbor.load_component should make sure that the expressions in the file are parsable. Checking the semantic correctness of this call should be covered by Arbor's test suite already.

To check the examples, you could add an arbor.load_component call in the check_acc_dir function for non-JSON files. I.e. after this line:

assert ref_file == test_file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants