Skip to content

Comments

fix bug which happens when .mat data contains a sparse matrix in COO#23

Merged
jornbr merged 3 commits intoBoldingBruggeman:mainfrom
ruiying-ocean:debug
Sep 11, 2025
Merged

fix bug which happens when .mat data contains a sparse matrix in COO#23
jornbr merged 3 commits intoBoldingBruggeman:mainfrom
ruiying-ocean:debug

Conversation

@ruiying-ocean
Copy link
Contributor

when fabmos (v 1.0.1 from conda-forge) uses a old version of TMM (MITgcm_ECCO), a bug occurs because the .mat data has COO (coordinate) format, but the Python code is expecting a CSR (compressed sparse row) format. This PR converts the format to avoid the bug.

Traceback (most recent call last):
  File "/Users/vhf24tbu/tmm/MITgcm_ECCO/PISCES.py", line 18, in <module>
    sim = fabmos.transport.tmm.Simulator(domain, calendar=calendar, fabm=fabm_yaml)
  File "/Users/vhf24tbu/.local/share/mamba/envs/fabmos/lib/python3.13/site-packages/fabmos/transport/tmm.py", line 480, in __init__
    self.Aexp_src = TransportMatrix(
                    ~~~~~~~~~~~~~~~^
        Aexp_files,
        ^^^^^^^^^^^
    ...<6 lines>...
        order=order,
        ^^^^^^^^^^^^
    )
    ^
  File "/Users/vhf24tbu/.local/share/mamba/envs/fabmos/lib/python3.13/site-packages/fabmos/transport/tmm.py", line 129, in __init__
    self._get_matrix_metadata(file_list[0], order=order)
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vhf24tbu/.local/share/mamba/envs/fabmos/lib/python3.13/site-packages/fabmos/transport/tmm.py", line 223, in _get_matrix_metadata
    A_ir = A.indices
           ^^^^^^^^^
AttributeError: 'coo_matrix' object has no attribute 'indices'

@jornbr
Copy link
Member

jornbr commented Sep 11, 2025

Many thanks for reporting this and exploring a potential solution. It seems that recent scipy changes (scipy/scipy#21905) causes the matlab v5 arrays to be returned as coo_array, while previously they were returned as csc_matrix. That does mean, however, that the solution is to explicitly cast to CSC format, rather than the CSR format you propose here. In order to not depend further on details of scipy's sparse matrix implementation, I'd propose to add

A = scipy.sparse.csc_array(A)

just underneath

A = vardict[self._group_name]

That should address your issue and also maintain compatibilty with older scipy versions (in which case the cast does nothing). Could you test if that works for you, and if so, update your PR?

@jornbr
Copy link
Member

jornbr commented Sep 11, 2025

Actually a cleaner code could likely just use

A = vardict[self._group_name].tocsc()

@ruiying-ocean
Copy link
Contributor Author

Hi @jornbr, thanks for your rapid response!. your code is more efficient and it resolves my issue. I have updated the PR.

# MATLAB < 7.3 format
vardict = scipy.io.loadmat(fn, variable_names=(self._group_name,))
A = vardict[self._group_name]
## Convert COO matrix to CSR format if needed
Copy link
Member

Choose a reason for hiding this comment

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

CSR here should become CSC (or the comment could go, since the streamlined code is now pretty clear)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely! this is corrected.

@jornbr jornbr merged commit 26fa336 into BoldingBruggeman:main Sep 11, 2025
20 checks passed
@jornbr
Copy link
Member

jornbr commented Sep 11, 2025

Perfect - thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants