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

Consistency in Naming #2239

Open
thorstenhater opened this issue Dec 6, 2023 · 7 comments
Open

Consistency in Naming #2239

thorstenhater opened this issue Dec 6, 2023 · 7 comments
Labels
interface Anything user-facing, including public C++ and Python APIs.

Comments

@thorstenhater
Copy link
Contributor

thorstenhater commented Dec 6, 2023

Currently we have a few hooplas in C++/Python API consistency:

Current clamps

i_clamp ./. iclamp: Either the C++ or the Python member needs to be renamed.

In Python iclamp(1, 2, 3) will give a 3nA current starting at 1ms and ending at 3ms. In
C++ i_clamp(1, 2, 3) will result in a 1nA current with a sine profile of 2kHz phase shifted
by 3rad. Python's behaviour is handled by i_clamp::box in C++. Clearly, this needs to change.

Morphology

Some morphology primitive have the m prefix, some not.

Things to consider

  • remove the m prefix, ie mcable -> cable
  • add it to Python consistently
  • introduce a Python submodule for morphology types
  • make all primitives interchangeable with tuples, ie cable { branch, from, to} <> (branch, from, to)
@thorstenhater thorstenhater added bug interface Anything user-facing, including public C++ and Python APIs. labels Dec 6, 2023
@thorstenhater
Copy link
Contributor Author

@Helveg maybe something for you?

@Helveg
Copy link
Collaborator

Helveg commented Dec 6, 2023

make all primitives interchangeable with tuples, ie cable { branch, from, to} <> (branch, from, to)

I kind of like that arbor is slightly terse there. It's very consistent and explicit. If you allow this interchangability I think it will mostly just increase the cognitive load. What advantage do you see from it?

@Helveg maybe something for you?

I can try, but if my IDE's refactor button doesn't find an occurence, neither will I ;)

@thorstenhater
Copy link
Contributor Author

thorstenhater commented Dec 6, 2023

We do allow this conversion for mpoint, so, consistency. But I agree, explicit beats implicit.

rg -t cpp 'class_<m' python/
should turn up all the candidates ;)

@thorstenhater
Copy link
Contributor Author

Added an even more annoying example.

@halfflat
Copy link
Contributor

halfflat commented Dec 7, 2023

My vague voice from beyond says: I'd like to keep the 'm' prefix because the terms segment etc. are so overloaded in the literature that leaving the 'm' off invites more confusion. But with iclamp in particular, we should offer the C++ functionality in the Python API, ideally with the same names, and definitely with the same parameter orders.

@thorstenhater
Copy link
Contributor Author

Hey @halfflat,

thanks for the input. I agree on the conceptual overload and the need for disambiguation. These items could
alternatively live in arb::morph, making opting out of the prefix a possibility (and in Python arbor.morph.
Main point here though is consistency. From your and Robin's comment I think the consensus would be to use
the m prefix consistently in both Python and C++.

About i_clamp: I'd personally prefer to adjust the C++ API to the Python API, reasoning here is that we have a
lot more models in Python than in C++. And some tests were actually using the Python style c'tors in the incorrect
belief (comment!) they were identical. Ideal solution here would be this

// NB. consistent naming with Python
iclamp(mA amplitude, Hz freq=0, rad=phase);
iclamp(ms from, ms duration, mA amplitude, Hz freq=0, rad phase=0);
iclamp([(ms, mA)] envelope, Hz freq=0, rad phase=0);

Problem being, the first and second c'tors are ambiguous, so this would be my compromise:

// NB. consistent naming with Python
iclamp(mA amplitude);
iclamp(ms from, ms duration, mA amplitude, Hz freq=0, rad phase=0);
iclamp([(ms, mA)] envelope, Hz freq=0, rad phase=0);

although still a bit miffy. If I were designing this from scratch, I'd put the amplitude first 🤷.
Python is a bit easier

iclamp(ms from=0, ms duration=forever, mA amplitude=0, **, Hz freq=0, rad phase=0)
iclamp([(ms, mA)] envelope, **, Hz freq=0, rad phase=0)

Again, I'd love to put the amplitude first, but that'd break too much existing code in my book.

@halfflat
Copy link
Contributor

halfflat commented Dec 7, 2023

If we're aligning the APIs, we're going to have a version incompatibility regardless; perhaps it is better to break both in a way that will fail to compile (C++) or raise an exception if the old API is used?

We could throw out the i_clamp::i_clamp overloads and use factories such as i_clamp::box (Python equivalent i_clamp_box?), i_clamp::constant (or maybe steady) and i_clamp::envelope. Then we could consistently have amplitude first, for example. Instead of factories, could add a tag type to i_clamp ctor or overload the ctor with different parameter structs, keeping things typey and reducing the risk of parameter order mixups (e.g. i_clamp::i_clamp(const i_clamp_envelope&), i_clamp::i_clamp(const i_clamp_box&) etc.).

As things stand, I think the C++ API is currently the saner of the two though.

@thorstenhater thorstenhater mentioned this issue Jan 2, 2024
13 tasks
@thorstenhater thorstenhater removed the bug label Jan 4, 2024
thorstenhater added a commit that referenced this issue Jan 19, 2024
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://github.com/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:
- [X] simulation
- [X] schedule/generator
- [x] paintables
- [X] placeables
  - [X] iclamp
  - [X] threshold
- [X] connections
- [X] 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.
- [x] ~~rename py::iclamp OR cpp::i_clamp for consistency~~ covered by
#2239
- [x] use scale/base for iexpr paintables for consistency with
scaled_mech
- [x] ~~Use CBRNG for Poisson schedule~~ covered by #2243 
- [ ] Automate stub generation. A wishlist item, requires installing
extra software.
- [x] Properly integrate units w/ spack. NB. Units doesn't have a
spackage.

Closes #1983 
Closes #2032

---------

Co-authored-by: boeschf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interface Anything user-facing, including public C++ and Python APIs.
Projects
None yet
Development

No branches or pull requests

3 participants