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

Templatize on the need for the integral of the third kind #2362

Merged
merged 4 commits into from
Oct 25, 2019

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Oct 23, 2019

This makes it possible to compute F and E without computing J when Π is not needed.

Before:

Run on (4 X 2808 MHz CPU s)
CPU Caches:
  L1 Data 32K (x2)
  L1 Instruction 32K (x2)
  L2 Unified 262K (x2)
  L3 Unified 4194K (x1)
------------------------------------------------------
Benchmark               Time           CPU Iterations
------------------------------------------------------
BM_EllipticF          233 ns        228 ns    2951600
BM_EllipticFEΠ        235 ns        230 ns    2992000

After:

Run on (4 X 2808 MHz CPU s)
CPU Caches:
  L1 Data 32K (x2)
  L1 Instruction 32K (x2)
  L2 Unified 262K (x2)
  L3 Unified 4194K (x1)
------------------------------------------------------
Benchmark               Time           CPU Iterations
------------------------------------------------------
BM_EllipticF          133 ns        123 ns    4072800
BM_EllipticFEΠ        239 ns        230 ns    2992000


std::mt19937_64 random(42);
std::uniform_real_distribution<> distribution_φ(0.0, π / 2);
std::uniform_real_distribution<> distribution_n(0.0, 1.0);
Copy link
Member

Choose a reason for hiding this comment

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

We don't need the n's and the loop at line 40 is probably leading to too efficient caching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

};

template<typename T, typename = EnableIfAngleResult<T>>
inline constexpr bool should_compute = !std::is_same_v<T, UnusedResult const>;
Copy link
Member

Choose a reason for hiding this comment

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

Do we believe that template variables work on Mac?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work.

template<typename T, typename = EnableIfAngleResult<T>>
inline constexpr bool should_compute = !std::is_same_v<T, UnusedResult const>;

inline constexpr UnusedResult unused{uninitialized};
Copy link
Member

Choose a reason for hiding this comment

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

Same question for inline variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same answer.

template<typename T>
using EnableIfAngleResult =
std::enable_if_t<std::is_same_v<T, UnusedResult const> ||
std::is_same_v<T, Angle>>;
Copy link
Member

Choose a reason for hiding this comment

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

I'd reorder things a bit for ease of readability:

  1. UnusedResult,
  2. unused,
  3. EnableIfAngleResult,
  4. should_compute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. That also makes it legal, MSVC is overly lenient as usual.

Sorry, something went wrong.

Angle& B_φǀm,
Angle& D_φǀm,
Angle& J_φ_nǀm) {
return FukushimaEllipticBDJ<Angle>(φ, n, mc, B_φǀm, D_φǀm, J_φ_nǀm);
Copy link
Member

Choose a reason for hiding this comment

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

Is the specification of Angle necessary? I would have expected that it would deduce it.

Sorry, something went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would, but the non-templatized FukushimaEllipticBDJ (which we are defining) has precedence here.

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative would be to expose UnusedResult in base, and to only have the templatized version (declared in the header, with both explicit instantiation declarations).

F_φǀm = B + D;
E_φǀm = B + mc * D;
Π_φ_nǀm = F_φǀm + n * J;
EllipticFEΠ<Angle>(φ, n, mc, F_φǀm, E_φǀm, Π_φ_nǀm);
Copy link
Member

Choose a reason for hiding this comment

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

Same question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same answer.

@pleroy pleroy added the LGTM label Oct 25, 2019
@eggrobin eggrobin merged commit 25cf2fb into mockingbirdnest:master Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants