Skip to content
This repository has been archived by the owner on Dec 22, 2022. It is now read-only.

[BUGFIX] Order of values passed to csc.set #63

Merged
merged 1 commit into from
Apr 29, 2021
Merged

Conversation

bkj
Copy link
Contributor

@bkj bkj commented Apr 29, 2021

Order of values passed to csc.set is incorrect.

More descriptive names than Aj, Ap, Ax might be helpful here.

Likewise, more descriptive names thatn Ap, J, X, I and Aj would be helpful in graph/build/detail.hxx. Any reason not to use the following?

row_offsets
column_indices
nonzero_values
row_indices
column_offsets

Or ... in my opinion even better because they're shorter, the same length 😄 and nonzero_values is redundant:

row_offsets
col_indices
values
row_indices
col_offsets

Order of values passed to `csc.set` is incorrect.  

More descriptive names than `Aj`, `Ap`, `Ax` might be helpful here.  

Likewise, more descriptive names thatn `Ap`, `J`, `X`, `I` and `Aj` would be helpful in `graph/build/detail.hxx`.  Any reason not to use the following?
```
row_offsets
column_indices
nonzero_values
row_indices
column_offsets
```

Or ... in my opinion even better because they're shorter, the same length 😄 and `nonzero_values` is redundant:
```
row_offsets
col_indices
values
row_indices
col_offsets
```
@neoblizz
Copy link
Member

Or ... in my opinion even better because they're shorter, the same length 😄 and nonzero_values is redundant:

row_offsets
col_indices
values
row_indices
col_offsets

We can use these! If you wanna commit the name changes to this PR before we merge it, please go ahead!

@neoblizz neoblizz changed the base branch from master to dev April 29, 2021 04:27
@neoblizz neoblizz added 🐛 bug Something isn't working 🏡 api API related changes, suggestions, issues. labels Apr 29, 2021
@bkj
Copy link
Contributor Author

bkj commented Apr 29, 2021

Cleaned up names in this commit: #64

@neoblizz neoblizz merged commit c306799 into dev Apr 29, 2021
@neoblizz neoblizz deleted the bugfix/csc_construction branch April 30, 2021 22:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🏡 api API related changes, suggestions, issues. 🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants