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

Orthogonal decomposition #2783

Merged
merged 8 commits into from
Nov 26, 2020

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Nov 8, 2020

It looks like this speeds up AnalyticalSeriesTest.CompactRepresentation between 10% and 20%.

Copy link
Member

@pleroy pleroy left a comment

Choose a reason for hiding this comment

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

Strangely, the unit tests fail with major accuracy changes. This must be investigated.

PoissonSeriesBasisGenerator<
Series,
dimension,
degree,
Copy link
Member

Choose a reason for hiding this comment

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

Put all 3 parameters on a single line, similar to line 98. In general, clang-format doesn't do a great job with these templates...

Copy link
Member

Choose a reason for hiding this comment

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

Same formatting comment, see line 169.

PoissonSeriesBasisGenerator<
Series,
dimension,
degree,
Copy link
Member

Choose a reason for hiding this comment

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

Same.

Copy link
Member

Choose a reason for hiding this comment

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

Same.

@pleroy pleroy added the LGTM label Nov 8, 2020
@eggrobin eggrobin closed this Nov 15, 2020
@eggrobin eggrobin reopened this Nov 22, 2020
@pleroy pleroy removed the LGTM label Nov 22, 2020
numerics/frequency_analysis_body.hpp Show resolved Hide resolved
PoissonSeriesSubspace(PoissonSeriesSubspace&&) = default;

PoissonSeriesSubspace& operator=(PoissonSeriesSubspace const&) = default;
PoissonSeriesSubspace& operator=(PoissonSeriesSubspace&&) = default;
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that these 4 declarations could be removed.

numerics/poisson_series_basis.hpp Outdated Show resolved Hide resolved
PoissonSeriesBasisGenerator<
Series,
dimension,
degree,
Copy link
Member

Choose a reason for hiding this comment

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

Same formatting comment, see line 169.

std::index_sequence<indices...>>::Subspaces(Instant const& origin) {
return {PoissonSeriesSubspace{
static_cast<PoissonSeriesSubspace::Coordinate>(indices % dimension),
// The degree of the ith polynomial is i / dimension, so its parity is
Copy link
Member

Choose a reason for hiding this comment

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

There is no i, it's called indices.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this sentence is illegible if expressed in terms of a pack.

PoissonSeriesBasisGenerator<
Series,
dimension,
degree,
Copy link
Member

Choose a reason for hiding this comment

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

Same.

numerics/poisson_series_basis_body.hpp Outdated Show resolved Hide resolved
numerics/poisson_series_basis_body.hpp Outdated Show resolved Hide resolved
@pleroy pleroy added the LGTM label Nov 23, 2020
@pleroy
Copy link
Member

pleroy commented Nov 23, 2020

I can haz tests?

@pleroy pleroy removed the LGTM label Nov 23, 2020
@pleroy pleroy added the LGTM label Nov 25, 2020
@eggrobin eggrobin merged commit d797277 into mockingbirdnest:master Nov 26, 2020
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