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

Better handling of powers in modcc. #2061

Merged
merged 6 commits into from
Dec 21, 2022

Conversation

thorstenhater
Copy link
Contributor

  • $x^{-1} \Rightarrow 1/x\quad \forall x$
  • $x^n \Rightarrow x\cdot \dots \cdot x \quad x\in N; |x| < 5$
  • $x^n \Rightarrow 1/(x\cdot \dots \cdot x) \quad x\in N; |x| < 5; x < 0$
  • $b^e \Rightarrow \exp(\log(b) e)\quad \forall b, e$

The last point introduces potential errors when pow(b, e) is allowed, but log(b) is
undefined. These occur exactly when all of the following is true

  • $b < 0$
  • $e\in N$
  • $e$, $b$ not known at compile time (since we cover these cases before)

@llandsmeer
Copy link
Contributor

Just tried a build, got this:

FAILED: arbor/CMakeFiles/arbor.dir/__/mechanisms/generated/default/kdrmt_cpu.cpp.o
ccache /usr/bin/c++ -Darbor_EXPORTS_STATIC -I/home/llandsmeer/repos/pr/2061/ext/random123/include -I/home/llandsmeer/repos/pr/2061/arbor -I/home/llandsmeer/repos/pr/2061/arbor/include -I/home/llandsmeer/repos/pr/2061/build/arbor/include -O3 -DNDEBUG -fPIC -Wall -Wno-maybe-uninitialized -Wno-psabi -march=native -fvisibility=hidden -std=c++17 -MD -MT arbor/CMakeFiles/arbor.dir/__/mechanisms/generated/default/kdrmt_cpu.cpp.o -MF arbor/CMakeFiles/arbor.dir/__/mechanisms/generated/default/kdrmt_cpu.cpp.o.d -o arbor/CMakeFiles/arbor.dir/__/mechanisms/generated/default/kdrmt_cpu.cpp.o -c /home/llandsmeer/repos/pr/2061/build/mechanisms/generated/default/kdrmt_cpu.cpp
/home/llandsmeer/repos/pr/2061/build/mechanisms/generated/default/kdrmt_cpu.cpp: In function ‘void arb::default_catalogue::kernel_kdrmt::advance_state(arb_mechanism_ppack*)’:
/home/llandsmeer/repos/pr/2061/build/mechanisms/generated/default/kdrmt_cpu.cpp:155:40: error: no matching function for call to ‘log(double&)’
  155 |         assign(qt, S::exp(S::mul(S::log(_pp_var_q10), S::mul( (double)0.10000000000000001, S::sub(celsius,  (double)24.0)))));
      |                                  ~~~~~~^~~~~~~~~~~~~

@thorstenhater
Copy link
Contributor Author

Yes, that's due to the following chain of events (NB. It also fails in CI for the same reasons)
There's no Simd::log(Simd::scalar_t) since ARB_UNARY_ARTHIMETIC doesn't define scalar versions
(BINARY does, though). You cannot simply add it since the mapping scalar_t -> vector_t is non-injective
and C++ barfs 'cannot figure out template argument T' back at you. For BINARY that's not an issue, since
the second arg fixes the vector type.

I am currently thinking on how to add that without too much hassle.

@llandsmeer
Copy link
Contributor

Can confirm the build works. Runtime for the L5PC s 4.410, a 3.3x improvement.

llandsmeer
llandsmeer previously approved these changes Dec 12, 2022
@llandsmeer llandsmeer dismissed their stale review December 12, 2022 15:12

Didn't check GH actions - rerunning now

@llandsmeer
Copy link
Contributor

llandsmeer commented Dec 12, 2022

Symbolic differentiation test symbolic_pdiff.nonlinear, ../test/unit-modcc/test_symdiff.cpp:210: expressions: a^x; log(a)*a^x seems to have broken. Makes sense probably if we edit the definition of a power

@thorstenhater
Copy link
Contributor Author

Yes, that's because we compare strings here

@thorstenhater
Copy link
Contributor Author

There, should be fixed now.

- use std::Nan
- add more tests
- clean-up simplifier
  - remove subsumed branches
  - add a special case for x^y | x < 0
  - fix a bug
@thorstenhater thorstenhater merged commit fbd4118 into arbor-sim:master Dec 21, 2022
@thorstenhater thorstenhater deleted the modcc/better-powers branch December 21, 2022 12:13
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.

3 participants