python upgrade to use memory views #9
Conversation
Reviewer's Guide by SourceryThis pull request modernizes the codebase by upgrading to use memory views in Cython and making setup.py compatible with pyproject.toml. The changes include code formatting improvements, memory view syntax updates in Cython code, and build system modernization. Class diagram for updated Cython memory view usageclassDiagram
class CyEdfReader {
- size_t nsamples_per_record
+ size_t nsamples_per_record
+ read_digital_signal(int signalnum, int start, int n, np.int32_t[:] sigbuf)
+ read_phys_signal(int signalnum, int start, int n, np.float64_t[:] sigbuf)
+ load_phys_datarecord(np.float64_t[:] db, int n=0)
}
class EdfWriter {
+ blockWriteDigitalSamples(np.int32_t[:] data)
+ blockWriteDigitalShortSamples(np.int16_t[:] data)
}
class EdfReader {
+ __init__(str file_name, str annotations_mode="all")
}
class Edfinfo {
+ __init__(str file_name, str text_encoding=DEFAULT_TEXT_ENCODING)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cleemesser - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider keeping the version constraints from setup.py in pyproject.toml to ensure compatibility - particularly the numpy<2 constraint which was removed
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| It still more needs tests, more refractoring to make a | ||
| real pythonic api before heading to towards a polished package. | ||
| real pythonic api before heading to towards a polished package. |
There was a problem hiding this comment.
suggestion (typo): Remove duplicate word 'to'
The phrase 'to towards' contains a duplicate word - it should be either 'heading to' or 'heading towards'
| real pythonic api before heading to towards a polished package. | |
| real pythonic api before heading towards a polished package. |
| To upload to pypi:: | ||
| python -m build | ||
|
|
||
| twine upload -r legacypypi dist/* <- fix this> |
There was a problem hiding this comment.
suggestion: Clean up the command line instruction
The '<- fix this>' comment should either be removed or replaced with a proper explanation of what needs to be fixed
| twine upload -r legacypypi dist/* <- fix this> | |
| twine upload -r legacypypi dist/* |
| from pylab import * # get rid of this in future | ||
|
|
||
|
|
||
| def mfreqz(b, a=1): |
There was a problem hiding this comment.
issue (complexity): Consider separating the frequency response calculations and plotting into separate functions.
The functions mix signal processing calculations with visualization code. This coupling makes the code less reusable and harder to test. Consider splitting into separate calculation and plotting functions:
def calc_freq_response(b, a=1):
w, h = signal.freqz(b, a)
h_dB = 20 * log10(abs(h))
h_Phase = unwrap(arctan2(imag(h), real(h)))
return w, h_dB, h_Phase
def plot_freq_response(w, h_dB, h_Phase):
subplot(211)
plot(w/max(w), h_dB)
ylim(-150, 5)
ylabel('Magnitude (db)')
xlabel(r'Normalized Frequency (x$\pi$rad/sample)')
title(r'Frequency response')
subplot(212)
plot(w/max(w), h_Phase)
ylabel('Phase (radians)')
xlabel(r'Normalized Frequency (x$\pi$rad/sample)')
title(r'Phase response')
subplots_adjust(hspace=0.5)
show()
# Convenience wrapper maintaining original functionality
def mfreqz(b, a=1):
w, h_dB, h_Phase = calc_freq_response(b, a)
plot_freq_response(w, h_dB, h_Phase)This approach:
- Separates concerns between calculation and visualization
- Enables reuse of calculations without plotting
- Makes the code more testable
- Maintains the original convenient interface
| if len(kwargs) > 0: | ||
| raise Exception('Unhandled argument(s) given: %r' % list(kwargs.keys())) | ||
| raise Exception("Unhandled argument(s) given: %r" % list(kwargs.keys())) |
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| ylabel("Magnitude (db)") | ||
| xlabel(r"Normalized Frequency (x$\pi$rad/sample)") | ||
| title(r"Frequency response") | ||
| subplot(212) | ||
| h_Phase = unwrap(arctan2(imag(h), real(h))) | ||
| plot(w / max(w), h_Phase) | ||
| ylabel("Phase (radians)") | ||
| xlabel(r"Normalized Frequency (x$\pi$rad/sample)") | ||
| title(r"Phase response") |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
| stem(x, response) | ||
| ylabel("Amplitude") | ||
| xlabel(r"n (samples)") | ||
| title(r"Impulse response") | ||
| subplot(212) | ||
| step = cumsum(response) | ||
| stem(x, step) | ||
| ylabel("Amplitude") | ||
| xlabel(r"n (samples)") | ||
| title(r"Step response") |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
| cekgtr = cekg[:len(ekgtr)] | ||
| fekgtr = ekgtr[0:len(cekgtr)]-cekgtr[:] | ||
| cekgtr = cekg[: len(ekgtr)] | ||
| fekgtr = ekgtr[0 : len(cekgtr)] - cekgtr[:] |
There was a problem hiding this comment.
suggestion (code-quality): Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] (remove-redundant-slice-index)
| fekgtr = ekgtr[0 : len(cekgtr)] - cekgtr[:] | |
| fekgtr = ekgtr[:len(cekgtr)] - cekgtr[:] |
|
|
||
| # findx2 = [ (ii, signal_labels[ii]) for ii in range(len(signal_labels))] | ||
| # findx2 = [ (ii, signal_labels[ii]) for ii in range(len(signal_labels))] | ||
| # X2 is signal 27 | ||
| # A2 is signal 23 | ||
|
|
||
| ekg = sigbufs[27][0:l] - sigbufs[23][0:l] |
There was a problem hiding this comment.
issue (code-quality): Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] [×3] (remove-redundant-slice-index)
| if False: | ||
| # try pickout signal 5 | ||
| _edflib.rewind(ef.handle, 5) | ||
| _edflib.read_int_samples(ef.handle, 5, Nibuf,ibuf) | ||
| _edflib.read_int_samples(ef.handle, 5, Nibuf, ibuf) |
There was a problem hiding this comment.
suggestion (code-quality): Remove redundant conditional (remove-redundant-if)
| if False: | |
| # try pickout signal 5 | |
| _edflib.rewind(ef.handle, 5) | |
| _edflib.read_int_samples(ef.handle, 5, Nibuf,ibuf) | |
| _edflib.read_int_samples(ef.handle, 5, Nibuf, ibuf) |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This switches to using more modern memory views interface to numpy in Cython. upgrades to use Cython 3.0+ fixed setup.py to work with pyproject.toml by removing conflicting info
Summary by Sourcery
Upgrade the project to use memory views with numpy in Cython, enhancing performance and compatibility. Update the build configuration by removing conflicting setup.py information to align with pyproject.toml. Upgrade to Cython 3.0+ to leverage new features and improvements.
New Features:
Enhancements:
Build: