Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adds Fourier coefficient calculation module #1160
Adds Fourier coefficient calculation module #1160
Changes from 65 commits
0111e13
97d4242
028f178
25dd3a4
6c51f4e
11e6d96
24755d1
8429c33
fa61e00
6ef1316
266bc59
5fed522
883d76f
2e26621
4cb28d8
0852c23
2c1e79b
4fef3e4
88d8e26
7498e33
cb01c5c
efa3109
ec47232
8355dc8
4086e50
f1fef40
7a4b8f0
e18b925
205735c
0dcdcd0
0a0f57f
42fbf93
bc1cb7a
bc802d5
5d3ad54
2dbe1a8
5045ec1
e0adf8b
2be1267
1ba1fce
3b4da13
c911693
d054cc2
e674b0a
80e58de
19714e7
4e4ed8a
9f63db8
62e6a01
a24d251
e370809
54bc263
67cd38a
b332221
3de4307
dc5fa7a
a53613b
c9b223f
191fcd8
4af9f2e
00b07f4
7f7088e
cec1acb
182d3ac
3913c6f
421a885
ae20b03
d597b0d
9d78989
b6ac07a
e2be80c
94cc3b1
e96cb3d
37639fa
e5dc33c
f85202b
0f0696c
26bb397
4dd6e85
7fbeea1
a8ac502
da9e607
e1695b6
978953d
58e7b23
f86f5cc
88005e3
2e8c512
00a5666
904ab07
63d5409
fbfd0fe
0a5770b
9bc5b06
dbd2482
e0ff851
b79b380
d81800e
c858c56
a5debd2
ab4cfa5
31d3522
d8e1229
c5a0d3b
ef1fb36
8261ca3
ffc0589
4305e98
b5b70d9
1e7de22
2a6ef9e
4455864
267745d
ebf4acf
d52471f
13f0ada
db1a6db
30b606f
9c8626d
a2b1a79
d87a34f
9d0bfc4
5025f3f
a18a032
821974d
c416bbf
0e70dd6
901bae0
ff897cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be nice if visualization was decoupled from calculation of Fourier coefficients (I can think of a few uses for knowing the coefficients but not wanting to visualize)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it already decoupled?
fourier_coefficients
is imported above thetry-except
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a different note, this should be a warning rather than a print statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not decoupled if you get a warning msg about matplotlib even if you are not planning to do visualization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can import
matplotlib
within the__init__.py
of the visualization submodule, rather than at the top-level moduleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommend shorter name (e.g., using
coeffs
instead ofcoefficients
), and considering an alternate name for thereconstruct_...
functions (why "re"construct?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, maybe we can also switch the order to
plot_coeffs_violin
,plot_coeffs_bar
.