-
Notifications
You must be signed in to change notification settings - Fork 2
[WIP] Fix segmentation fault in test_mfi_calculation #26
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
Conversation
|
@copilot continue |
…ting in test Co-authored-by: deepentropy <8287111+deepentropy@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Pull request overview
This PR addresses a segmentation fault that occurred during the test_mfi_calculation test by adding defensive memory layout checks to the MFI (Money Flow Index) function and updating the test to explicitly cast data types.
Key Changes:
- Added
np.ascontiguousarraywrappers to ensure contiguous memory layout in MFI function input arrays - Updated MFI test to explicitly cast OHLCV DataFrame columns to
np.float64before calling the function
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/numta/api/momentum_indicators.py |
Wrapped input array conversions with np.ascontiguousarray to ensure contiguous memory layout and prevent numba JIT segfaults |
tests/test_pandas_ext_comprehensive.py |
Added explicit np.float64 dtype casting to OHLCV columns in the MFI test to prevent type-related issues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| # Convert to numpy arrays with float64 dtype and ensure contiguous memory layout | ||
| # This prevents potential segfaults in numba JIT-compiled functions | ||
| high = np.ascontiguousarray(np.asarray(high, dtype=np.float64)) | ||
| low = np.ascontiguousarray(np.asarray(low, dtype=np.float64)) | ||
| close = np.ascontiguousarray(np.asarray(close, dtype=np.float64)) | ||
| volume = np.ascontiguousarray(np.asarray(volume, dtype=np.float64)) |
Copilot
AI
Dec 2, 2025
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.
The np.ascontiguousarray wrapper is only applied to the MFI function, but not to other similar volume-based indicators like AD, OBV, and ADOSC in src/numta/api/volume_indicators.py, which also use numba JIT compilation and accept the same types of inputs.
If non-contiguous arrays cause segfaults in MFI's numba functions, the same issue could potentially affect other indicators. Consider either:
- Applying this fix consistently to all similar numba-compiled functions that accept multiple array inputs, or
- Documenting why MFI specifically requires contiguous arrays while others don't
This ensures consistency and prevents similar issues from occurring with other indicators.
| df['high'] = df['high'].astype(np.float64) | ||
| df['low'] = df['low'].astype(np.float64) | ||
| df['close'] = df['close'].astype(np.float64) | ||
| df['volume'] = df['volume'].astype(np.float64) |
Copilot
AI
Dec 2, 2025
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.
The explicit dtype casting here is redundant. The pandas accessor's _get_column method (line 119 in src/numta/pandas_ext.py) already converts DataFrame column values to np.float64 using .values.astype(np.float64). Additionally, the MFI function itself now ensures correct dtype conversion with np.asarray(..., dtype=np.float64).
This redundant casting adds unnecessary overhead and makes the test inconsistent with other volume indicator tests (test_obv_calculation and test_ad_calculation) which don't perform explicit casting.
| df['high'] = df['high'].astype(np.float64) | |
| df['low'] = df['low'].astype(np.float64) | |
| df['close'] = df['close'].astype(np.float64) | |
| df['volume'] = df['volume'].astype(np.float64) |
| high = np.ascontiguousarray(np.asarray(high, dtype=np.float64)) | ||
| low = np.ascontiguousarray(np.asarray(low, dtype=np.float64)) | ||
| close = np.ascontiguousarray(np.asarray(close, dtype=np.float64)) | ||
| volume = np.ascontiguousarray(np.asarray(volume, dtype=np.float64)) |
Copilot
AI
Dec 2, 2025
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.
The double wrapping np.ascontiguousarray(np.asarray(...)) is inefficient. Consider using np.ascontiguousarray directly with the dtype parameter instead:
high = np.ascontiguousarray(high, dtype=np.float64)
low = np.ascontiguousarray(low, dtype=np.float64)
close = np.ascontiguousarray(close, dtype=np.float64)
volume = np.ascontiguousarray(volume, dtype=np.float64)This avoids creating an intermediate array when the input is not already a numpy array, improving performance while achieving the same result.
| high = np.ascontiguousarray(np.asarray(high, dtype=np.float64)) | |
| low = np.ascontiguousarray(np.asarray(low, dtype=np.float64)) | |
| close = np.ascontiguousarray(np.asarray(close, dtype=np.float64)) | |
| volume = np.ascontiguousarray(np.asarray(volume, dtype=np.float64)) | |
| high = np.ascontiguousarray(high, dtype=np.float64) | |
| low = np.ascontiguousarray(low, dtype=np.float64) | |
| close = np.ascontiguousarray(close, dtype=np.float64) | |
| volume = np.ascontiguousarray(volume, dtype=np.float64) |
np.ascontiguousarrayChanges
src/numta/api/momentum_indicators.pynp.ascontiguousarraywrapper around input arrays in MFI function to ensure contiguous memory layout, which prevents potential segfaults in numba JIT-compiled functionstests/test_pandas_ext_comprehensive.pytest_mfi_calculationto explicitly cast OHLCV columns tonp.float64dtype before calling the MFI functionOriginal prompt
This pull request was created as a result of the following prompt from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.