signal.py and bdot_probe.py #1652
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
svincena
reviewed
Jul 29, 2022
plasmapy/analysis/signal.py
Outdated
| data_array[:] = [x - meansub for x in data_array] | ||
| return data_array | ||
|
|
||
| def band_pass_filter(data_array: np.ndarray, cutoff: float, sampling_frequency: float, order: int) -> np.ndarray: |
There was a problem hiding this comment.
This function implements a high-pass filter, not a band-pass filter. Consider renaming this function "high_pass_filter" or implement the high and low cutoffs
rocco8773
requested changes
Aug 1, 2022
There was a problem hiding this comment.
Thank you for working on this. I've left a few comments for changes.
How comfortable are you with writing tests? We'll need to cover the functionality being introduced here. @namurphy Has put together a rather robust testing guide that you can use for reference.
plasmapy/analysis/bdot_probe.py
Outdated
| @@ -0,0 +1,31 @@ | |||
| """_summary_ | |||
There was a problem hiding this comment.
Remove this file from this pull request (PR). We'll include this in its own PR.
plasmapy/analysis/sig_analysis.py
Outdated
| @@ -0,0 +1,46 @@ | |||
| """_summary_ | |||
plasmapy/analysis/sig_analysis.py
Outdated
Comment on lines
12
to
20
| """_summary_ : Function which preforms mean subtraction | ||
|
|
||
| Args: | ||
| data_array (array): array containing values from which to mean sub | ||
| start_idx (int): idx of where to start obtaining mean from | ||
| end_idx (int): idx of where to stop obtaining mean from | ||
| Returns: | ||
| data_array (array): array after mean_sub is preformed | ||
| """ |
There was a problem hiding this comment.
We use the numpydoc style for our documentation. Can you please look into converting your docstrings?
You can also refer to our documentation guide.
|
Hi,
I changed the docstrings to the numpydoc style. If I
missed something, please let me know.
I do not know how to write tests but I can always learn. I will read up on
the testing guide!
Additonally, I can't change the name to signal.py because that causes a
circular import issue. Numpy already has a module called signal.py
Best,
Amina
…On Mon, 1 Aug 2022 at 15:40, Erik Everson ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thank you for working on this. I've left a few comments for changes.
How comfortable are you with writing tests? We'll need to cover the
functionality being introduced here. @namurphy
<https://github.com/namurphy> Has put together a rather robust testing
guide
<https://docs.plasmapy.org/en/stable/contributing/testing_guide.html>
that you can use for reference.
------------------------------
In plasmapy/analysis/bdot_probe.py
<#1652 (comment)>:
> @@ -0,0 +1,31 @@
+"""_summary_
Remove this file from this pull request (PR). We'll include this in its
own PR.
------------------------------
In plasmapy/analysis/sig_analysis.py
<#1652 (comment)>:
> @@ -0,0 +1,46 @@
+"""_summary_
Rename this file signal.py.
------------------------------
In plasmapy/analysis/sig_analysis.py
<#1652 (comment)>:
> + """_summary_ : Function which preforms mean subtraction
+
+ Args:
+ data_array (array): array containing values from which to mean sub
+ start_idx (int): idx of where to start obtaining mean from
+ end_idx (int): idx of where to stop obtaining mean from
+ Returns:
+ data_array (array): array after mean_sub is preformed
+ """
We use the numpydoc style
<https://numpydoc.readthedocs.io/en/latest/format.html> for our
documentation. Can you please look into converting your docstrings?
You can also refer to our documentation guide
<https://docs.plasmapy.org/en/stable/contributing/doc_guide.html>.
—
Reply to this email directly, view it on GitHub
<#1652 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AS6ICD7F76B5PDTC6IEKLJDVXAR33ANCNFSM55BNU7RA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
…nostics/magnetics.py
github-actions
bot
added
the
plasmapy.diagnostics
StanczakDominik
requested changes
Aug 3, 2022
plasmapy/analysis/sig_analysis.py
Outdated
| import numpy as np | ||
| import scipy.io as spio | ||
| import scipy.integrate as sp | ||
| from scipy import signal |
There was a problem hiding this comment.
Additonally, I can't change the name to signal.py because that causes a circular import issue. Numpy already has a module called signal.py
That's not technically a circular import issue. You can, since it's sort of necessary here, do
Suggested change
| from scipy import signal | |
| import scipy.signal |
And then you can refer to this later on as
b, a = scipy.signal.butter(order, normal_cutoff, btype='highpass', analog=False)
plasmapy/analysis/magnetics.py
Outdated
| if len(bdot_data) != len(times): | ||
| raise Exception("length of time and voltage arrays in not equal\n") | ||
|
|
||
| bdot_data[:] = [x / loop_area for x in bdot_data] |
There was a problem hiding this comment.
Suggested change
| bdot_data[:] = [x / loop_area for x in bdot_data] | |
| bdot_data /= loop_area |
As you wrote it, this would run the division via Python rather than numpy (slow!) and modify the array in-place, which is fine for performance reasons, but really, really, really needs to be explicitly stated in the documentation.
plasmapy/analysis/magnetics.py
Outdated
Comment on lines
40
to
41
| field_arr = sp.cumtrapz(bdot_data, times) # Tesla | ||
| new_time = times[1:] |
There was a problem hiding this comment.
I definitely agree with Steve here: #1648 (comment) that if we do initial = 0 in cumtrapz, we don't need to modify the times array and could skip the whole new_time thing.
plasmapy/analysis/sig_analysis.py
Outdated
Comment on lines
20
to
24
| start_idx: int | ||
| Index of value in array from where to start calulating offset. | ||
|
|
||
| end_idx: int | ||
| Index of value in array from where to end calulating offset. |
There was a problem hiding this comment.
Typos:
Suggested change
| start_idx: int | |
| Index of value in array from where to start calulating offset. | |
| end_idx: int | |
| Index of value in array from where to end calulating offset. | |
| start_idx: int | |
| Index of value in array from where to start calculating offset. | |
| end_idx: int | |
| Index of value in array from where to end calculating offset. |
plasmapy/analysis/sig_analysis.py
Outdated
| """ | ||
| data_array = np.array(data_array) | ||
| meansub = np.mean(data_array[start_idx:end_idx]) | ||
| data_array[:] = [x - meansub for x in data_array] |
There was a problem hiding this comment.
Same caveat as above, don't use loops for this!
plasmapy/analysis/sig_analysis.py
Outdated
| from warnings import warn | ||
|
|
||
|
|
||
| def remove_offset(data_array: np.ndarray, start_idx: int, end_idx: int) -> np.ndarray: |
There was a problem hiding this comment.
This whole function seems overly complicated for a two-liner:
offset_region = data_array[start_idx, end_idx]
data_array -= offset_region.mean()Perhaps we could skip it?
|
Hi,
Thank you for the feedback.
I made the suggested changes except for signal.py
I tried what you recommended: changing *from scipy import signal *to *import
scipy.signal *and using *scipy.signal.butter* instead of *signal.butter*
However, it gave me the following error:
Traceback (most recent call last):
File
"/Users/aminaahmed/Documents/Project_Plasma/PlasmaPy/plasmapy/analysis/signal.py",
line 4, in <module>
import numpy as np
File
"/Users/aminaahmed/opt/anaconda3/lib/python3.9/site-packages/numpy/__init__.py",
line 140, in <module>
from . import core
File
"/Users/aminaahmed/opt/anaconda3/lib/python3.9/site-packages/numpy/core/__init__.py",
line 100, in <module>
from . import _add_newdocs_scalars
File
"/Users/aminaahmed/opt/anaconda3/lib/python3.9/site-packages/numpy/core/_add_newdocs_scalars.py",
line 9, in <module>
import platform
File "/Users/aminaahmed/opt/anaconda3/lib/python3.9/platform.py", line
119, in <module>
import subprocess
File "/Users/aminaahmed/opt/anaconda3/lib/python3.9/subprocess.py", line
49, in <module>
import signal
File
"/Users/aminaahmed/Documents/Project_Plasma/PlasmaPy/plasmapy/analysis/signal.py",
line 5, in <module>
import scipy.io as spio
File
"/Users/aminaahmed/opt/anaconda3/lib/python3.9/site-packages/scipy/__init__.py",
line 71, in <module>
from numpy import __version__ as __numpy_version__
ImportError: cannot import name '__version__' from partially initialized
module 'numpy' (most likely due to a circular import)
(/Users/aminaahmed/opt/anaconda3/lib/python3.9/site-packages/numpy/__init__.py)
I didn't know what to do with the error so I discarded the changes. Please
let me know what you think.
Additionally, for the last comment:
*This whole function seems overly complicated for a two-liner:*
*offset_region = data_array[start_idx, end_idx]
data_array -= offset_region.mean()*
*Perhaps we could skip it?*
I didn't quite understand what you are saying. Should I change the code or
just completely remove the function?
It would be great if you could rephrase it so I can understand it better.
Best,
Amina
…On Wed, 3 Aug 2022 at 10:17, Dominik Stańczak ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In plasmapy/analysis/sig_analysis.py
<#1652 (comment)>:
> @@ -0,0 +1,75 @@
+"""_summary_
+"""
+
+import numpy as np
+import scipy.io as spio
+import scipy.integrate as sp
+from scipy import signal
Additonally, I can't change the name to signal.py because that causes a
circular import issue. Numpy already has a module called signal.py
That's not technically a circular import issue. You can, since it's sort
of necessary here, do
⬇️ Suggested change
-from scipy import signal
+import scipy.signal
And then you can refer to this later on as
b, a = scipy.signal.butter(order, normal_cutoff, btype='highpass', analog=False)
------------------------------
In plasmapy/analysis/magnetics.py
<#1652 (comment)>:
> +
+ loop_area: float
+ The area through which the changing flux is measured.
+
+ Returns
+ -------
+ field_arr: `numpy.ndarray`
+ The array containing the magnetic field fluctuations.
+
+ new_time: `numpy.ndarray`
+ The corresponding time series.
+ """
+ if len(bdot_data) != len(times):
+ raise Exception("length of time and voltage arrays in not equal\n")
+
+ bdot_data[:] = [x / loop_area for x in bdot_data]
⬇️ Suggested change
- bdot_data[:] = [x / loop_area for x in bdot_data]
+ bdot_data /= loop_area
As you wrote it, this would run the division via Python rather than numpy
(slow!) and modify the array in-place, which is fine for performance
reasons, but really, really, really needs to be explicitly stated in the
documentation.
------------------------------
In plasmapy/analysis/magnetics.py
<#1652 (comment)>:
> + field_arr = sp.cumtrapz(bdot_data, times) # Tesla
+ new_time = times[1:]
I definitely agree with Steve here: #1648 (comment)
<#1648 (comment)>
that if we do initial = 0 in cumtrapz, we don't need to modify the times
array and could skip the whole new_time thing.
------------------------------
In plasmapy/analysis/sig_analysis.py
<#1652 (comment)>:
> + start_idx: int
+ Index of value in array from where to start calulating offset.
+
+ end_idx: int
+ Index of value in array from where to end calulating offset.
Typos:
⬇️ Suggested change
- start_idx: int
- Index of value in array from where to start calulating offset.
-
- end_idx: int
- Index of value in array from where to end calulating offset.
+ start_idx: int
+ Index of value in array from where to start calculating offset.
+
+ end_idx: int
+ Index of value in array from where to end calculating offset.
------------------------------
In plasmapy/analysis/sig_analysis.py
<#1652 (comment)>:
> +
+ start_idx: int
+ Index of value in array from where to start calulating offset.
+
+ end_idx: int
+ Index of value in array from where to end calulating offset.
+
+ Returns
+ -------
+ data_array: `numpy.ndarray`
+ the original data array with the offset removed.
+
+ """
+ data_array = np.array(data_array)
+ meansub = np.mean(data_array[start_idx:end_idx])
+ data_array[:] = [x - meansub for x in data_array]
Same caveat as above, don't use loops for this!
------------------------------
In plasmapy/analysis/sig_analysis.py
<#1652 (comment)>:
> @@ -0,0 +1,75 @@
+"""_summary_
+"""
+
+import numpy as np
+import scipy.io as spio
+import scipy.integrate as sp
+from scipy import signal
+from warnings import warn
+
+
+def remove_offset(data_array: np.ndarray, start_idx: int, end_idx: int) -> np.ndarray:
This whole function seems overly complicated for a two-liner:
offset_region = data_array[start_idx, end_idx]
data_array -= offset_region.mean()
Perhaps we could skip it?
—
Reply to this email directly, view it on GitHub
<#1652 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AS6ICD4LQ5KMSY7HCC4ECMTVXJ5RBANCNFSM55BNU7RA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Hey, I've made a couple changes to your branch, please pull them via
Yeah, sorry, that's on me - communication, especially clear communication, can be difficult 😅 I do think that since this function can be expressed as a numpy one-liner, it isn't necessary and can be removed - which I did in 6e645ba. I also could not reproduce your |
|
Hi,
Sorry for the late reply. I am in a different country right now.
I still got the error with signal.py when I pulled the changes. However,
since you are not getting the error, I think the problem is with my
environment. I have had issues with it before so I don't think this one is
any different.
I'll try to figure out what is wrong at my end but since you are not
getting the error, I think the code is probably fine.
Best,
Amina
…On Tue, 16 Aug 2022 at 21:41, Dominik Stańczak-Marikin < ***@***.***> wrote:
Hey, I've made a couple changes to your branch, please pull them via git
pull :)
I didn't quite understand what you are saying. Should I change the code or
just completely remove the function? It would be great if you could
rephrase it so I can understand it better.
Yeah, sorry, that's on me - communication, especially clear communication,
can be difficult 😅 I do think that since this function can be expressed
as a numpy one-liner, it isn't necessary and can be removed - which I did
in 6e645ba
<6e645ba>
.
I also could not reproduce your signal-related error. Can you tell me
whether you're still getting it after pulling in the changes?
—
Reply to this email directly, view it on GitHub
<#1652 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AS6ICDZ52PQF3FNAUOFURPTVZPAEBANCNFSM55BNU7RA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
It does look like an env problem! Since from the previous error logs it looks like you're using anaconda, I'll point out the instructions over at https://docs.plasmapy.org/en/stable/contributing/install_dev.html#conda, ending at and including the |
namurphy
added
status: dormant
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
All functions do not use units.
All functions strictly use 1D numpy arrays.