[BUG] Handle optional preamble lines in DepmtxReader and pad csc indptr #534
Conversation
This file contains hidden or 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
sarapelka-blyk
changed the title
[BUG] Handle optional preamble lines in DepmtxReader and pad csc indptr
|
Thanks @sarapelka-blyk! And sorry for the delay. I'll get on this this week |
drewejohnson
requested changes
Oct 4, 2025
Comment on lines
+163
to
+174
| # Pad column pointer if file omits trailing all-zero columns | ||
| _, nCols = matrixSize | ||
| indptr = cscProcessor.indptr | ||
| expected = nCols + 1 | ||
| if indptr.shape[0] < expected: | ||
| pad = expected - indptr.shape[0] | ||
| indptr = concatenate([indptr, full(pad, indptr[-1], dtype=indptr.dtype)]) | ||
| elif indptr.shape[0] > expected: | ||
| raise ValueError(f"index pointer size ({indptr.shape[0]}) should be ({expected})") | ||
| self.depmtx = csc_matrix( | ||
| (cscProcessor.data[:, 0], cscProcessor.indices, | ||
| cscProcessor.indptr), dtype=longfloat, shape=matrixSize) | ||
| (cscProcessor.data[:, 0], cscProcessor.indices, indptr), | ||
| dtype=longfloat, shape=matrixSize) |
There was a problem hiding this comment.
This logic isn't immediately clear to me. If the shape of the matrix is known ahead of construction, does it matter that there are full zero columns? They shouldn't have any information in the sparse matrix structure.
Can you add some testing for this logic?
| BaseReader.__init__(self, filePath, 'depmtx') | ||
| SparseReaderMixin.__init__(self, sparse) | ||
| self.deltaT = None | ||
| self.flx = None |
There was a problem hiding this comment.
Similar comments from #512
- Please add this to the
Attributessection of the docstring, when it may exist or not, what purpose it serves, etc. - Please consider using the full name
self.flux
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes #533
Make sure you have read over the developer guide to ease
the review process. These include:
docs/contributors.rstInclude a thorough and concise overview of the changes, and why this PR is overall beneficial to the project.
The previous version of the reader errors out when it comes across a preamble/initialization of the N0, N1, A, and Z matrices. Additionally it cannot handle a normalization factor at the top of the file.
Changes made:
Handle optional preamble lines in DepmtxReader (flx, N0/ZAI/N1)
flx = ...normalization factorN0 = zeros(...),ZAI = zeros(...),N1 = zeros(...)initializersindptrwhen depmtx has trailing all-zero columnsDepmtxReader.flx