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

Ux/units #2233

Merged
merged 66 commits into from
Jan 19, 2024
Merged

Ux/units #2233

merged 66 commits into from
Jan 19, 2024

Conversation

thorstenhater
Copy link
Contributor

@thorstenhater thorstenhater commented Nov 28, 2023

The core issue here is to add units to the user facing API. I decided on using the LLNL/units
library, which offers conversion and checking at runtime. Runtime is a requirement -- as much
as I love static guarantees --, but keeping the interface uniform between Python and C++ is a
must.

While setting this up, I noticed the severe lack of IDE/LSP support for Arbor, so I added typing
stubs using https:/sizmailov/pybind11-stubgen. The conjunction of typing and units
exposed misuse of pybind11 in several places, so next I had to massage the ordering of bindings,
adjust the specification of default arguments, and add the odd missing binding.

The schedule/event generator interface was tightened up, hiding the *_impl structs and exposing
only the type erased schedule object. That in turn required de-generification of the Poisson
schedule. Now, Mersenne twister is the only choice and I will remove that later on for the CBRNG
we are already using elsewhere.

Currently, units are used for:

  • simulation
  • schedule/generator
  • paintables
  • placeables
    • iclamp
    • threshold
  • connections
  • gap junctions

Adding units to mechanism interfaces is interesting but requires more work and thought, so
I'll defer that to a later point in time. We'd need to adjust modcc to expose and check units
and devise a scheme to handle missing units.

Generic TODOs; some might spin off into separate issues.

  • rename py::iclamp OR cpp::i_clamp for consistency covered by Consistency in Naming #2239
  • use scale/base for iexpr paintables for consistency with scaled_mech
  • Use CBRNG for Poisson schedule covered by Use CBRNG for Schedules #2243
  • Automate stub generation. A wishlist item, requires installing extra software.
  • Properly integrate units w/ spack. NB. Units doesn't have a spackage.

Closes #1983
Closes #2032

@thorstenhater thorstenhater marked this pull request as ready for review January 8, 2024 10:23
@thorstenhater
Copy link
Contributor Author

Note to reviewer: Spack is broken on stable for some reason out of my control; Develop works fine, though.

Copy link
Contributor

@boeschf boeschf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round, haven't looked at everything yet. Massive PR, lots of work! thanks for tackling this.
Have you noticed any appreciable performance hits due to runtime unit conversion and checking?

ext/CMakeLists.txt Show resolved Hide resolved
arbor/include/arbor/units.hpp Outdated Show resolved Hide resolved
arbor/include/arbor/units.hpp Show resolved Hide resolved
arbor/include/arbor/cable_cell_param.hpp Outdated Show resolved Hide resolved
arbor/include/arbor/cable_cell_param.hpp Outdated Show resolved Hide resolved
arbor/include/arbor/event_generator.hpp Show resolved Hide resolved
arbor/fvm_layout.cpp Outdated Show resolved Hide resolved
@thorstenhater
Copy link
Contributor Author

Note about performance: This is only at the outer most 'shell' of our API. Everything below is
not unit-tagged. For that, I'd use -- if at all -- a compile time library (some choices available).
Here, we cannot, as we need to support Python's dynamic typing.

So, it comes down to constructing a class per such API call, some math ops, and a check. The most
expensive is explicit_schedule. All that is bounded by place, paint, and generators and runs at
model description time (not even construction inside Arbor).

thorstenhater and others added 3 commits January 13, 2024 17:07
- inline variables
- use precise_units
- add units to add_ion
- more checks for cable_cell_params
- remove spurious TODO
Copy link
Contributor

@boeschf boeschf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the second round. Expect the Python stuff in round 3. A couple general remarks:

  • what type is the expression1*U::ms converted to? It should be quantity but I'm unsure
  • the reformatting of many files makes the review tedious. we should start thinking about an automated way (clang-format, git pre-commit hooks, CI checks etc) in order to minimize changes

doc/index.rst Show resolved Hide resolved
using engine_type = std::mt19937_64;
using seed_type = std::remove_cv_t<decltype(engine_type::default_seed)>;

constexpr static auto default_seed = engine_type::default_seed;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constexpr static auto default_seed = engine_type::default_seed;
inline constexpr auto default_seed = engine_type::default_seed;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding a function which generates a random seed value, possibly using std::random_device?

Copy link
Contributor Author

@thorstenhater thorstenhater Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I am unsure whether I like that. The next step here should be to eradicate std::random and use r123 everywhere. Then seeding should be a function of cell gid and the relevant item's index. For example one would seed an event_generator with gid*100 + lid where lid is the running count of the generator in the list. We might consider giving users a convenience function/helper thing there to partially automate this.

In general, I refrain from this for reasons of reproducibility, since all kinds of ordering or reentrancy (word!) come into play. Thus, even now, I'd suggest using the gid + some offset as a seed in recipes.

arbor/schedule.cpp Outdated Show resolved Hide resolved
arbor/schedule.cpp Outdated Show resolved Hide resolved
arbor/schedule.cpp Outdated Show resolved Hide resolved
test/unit/test_lif_cell_group.cpp Show resolved Hide resolved
test/unit/test_s_expr.cpp Show resolved Hide resolved
test/unit/test_schedule.cpp Outdated Show resolved Hide resolved
test/unit/test_schedule.cpp Show resolved Hide resolved
test/unit/test_sde.cpp Show resolved Hide resolved
@thorstenhater
Copy link
Contributor Author

So this is the second round. Expect the Python stuff in round 3. A couple general remarks:

* what type is the expression`1*U::ms` converted to? It should be `quantity` but I'm unsure

Yes, it's a simple way to get a quantity from a unit.

* the reformatting of  many files makes the review tedious. we should start thinking about an automated way (clang-format, git pre-commit  hooks, CI checks etc) in order to minimize changes

Agreed, ten times over. I am also on record about not really caring about the actual style, just about consistency and automation.

Copy link
Contributor

@boeschf boeschf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

round 3

python/simulation.cpp Show resolved Hide resolved
python/simulation.cpp Show resolved Hide resolved
python/schedule.hpp Show resolved Hide resolved
python/schedule.cpp Outdated Show resolved Hide resolved
python/schedule.cpp Show resolved Hide resolved
doc/tutorial/calcium_stdp_curve.rst Outdated Show resolved Hide resolved
python/example/calcium_stdp.py Show resolved Hide resolved
python/example/probe_lfpykit.py Outdated Show resolved Hide resolved
python/example/single_cell_allen.py Outdated Show resolved Hide resolved
python/cells.cpp Outdated Show resolved Hide resolved
@boeschf
Copy link
Contributor

boeschf commented Jan 19, 2024

cscs-ci run daint-gpu

Copy link
Contributor

@boeschf boeschf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks

@thorstenhater thorstenhater merged commit 60445a4 into arbor-sim:master Jan 19, 2024
22 checks passed
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

Successfully merging this pull request may close these issues.

Reconsider fixed gpu spike buffer size Units! At least on the Python API surface
2 participants