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

fix: handle sparse matrix reading for big-endian byte order #163

Merged
merged 4 commits into from
Feb 21, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/elisa/data/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1138,8 +1138,14 @@ def __init__(
photon_egrid = np.array(photon_egrid, dtype=np.float64, order='C')
channel_emin = np.array(channel_emin, dtype=np.float64, order='C')
channel_emax = np.array(channel_emax, dtype=np.float64, order='C')
response_matrix = coo_array(response_matrix)
response_matrix.eliminate_zeros()
# if the byte order is not native,
# then convert it to be compatible with scipy.sparse
if not isinstance(response_matrix, sparray):
Copy link

Choose a reason for hiding this comment

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

question (bug_risk): Revisit the conditional conversion for response_matrix.

Currently, the conversion to native byte order is skipped if response_matrix is already an instance of sparray. Ensure that sparse matrices are always natively ordered or consider if they might also need conversion to avoid potential incompatibility issues with scipy.sparse.

if response_matrix.dtype.byteorder != '=':
order = response_matrix.dtype.newbyteorder()
response_matrix = response_matrix.astype(order)
response_matrix = coo_array(response_matrix)
response_matrix.eliminate_zeros()
channel = np.array(channel, dtype=str, order='C')

photon_egrid_shape = np.shape(photon_egrid)
Expand Down
6 changes: 6 additions & 0 deletions src/elisa/data/ogip.py
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,12 @@ def _read_response(self, file: str, response_id: int) -> dict:
else:
reduced_matrix = np.hstack(matrix)

# if the byte order is not native,
# then convert it to be compatible with scipy.sparse
if reduced_matrix.dtype.byteorder != '=':
order = reduced_matrix.dtype.newbyteorder()
reduced_matrix = reduced_matrix.astype(order)
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider centralizing duplicated byte order conversion logic.

The same pattern for converting non‐native byte order appears in both this file and in 'base.py'. Extracting this logic into a shared utility function could improve maintainability and reduce code duplication.


sparse_matrix = coo_array(
(reduced_matrix, (rows, cols)),
shape=(len(response_data), channel_number),
Expand Down
27 changes: 27 additions & 0 deletions tests/test_data.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import numpy as np
import pytest

from elisa.data.grouping import significance_gv, significance_lima
from elisa.data.ogip import Response, ResponseData
from elisa.models import PowerLaw


Expand Down Expand Up @@ -117,3 +119,28 @@ def test_data_grouping():
data.spec_counts, data.back_counts, data.back_errors, data.back_ratio
)
assert np.all(sig >= scale)


@pytest.mark.parametrize(
'file',
[
'../docs/notebooks/data/P011160500104_LE.rsp',
'../docs/notebooks/data/P011160500104_ME.rsp',
'../docs/notebooks/data/P011160500104_HE.rsp',
],
)
def test_load_response(file):
# Test Response against big-endian files
assert np.all(Response(file).channel_fwhm > 0)


def test_response():
# Test ResponseData against different endianness
photon_egrid = np.linspace(1.0, 100.0, 101)
channel = np.arange(100)
channel_emin = photon_egrid[:-1]
channel_emax = photon_egrid[1:]
matrix1 = np.eye(100).astype('<f4')
matrix2 = np.eye(100).astype('>f4')
ResponseData(photon_egrid, channel_emin, channel_emax, matrix1, channel)
ResponseData(photon_egrid, channel_emin, channel_emax, matrix2, channel)
Copy link

Choose a reason for hiding this comment

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

issue (testing): These tests are missing assertions.

Simply creating the ResponseData object doesn't verify its correctness. Add assertions to check that the data within the object is correctly handled for both little-endian and big-endian input matrices.

Loading