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

Surprising behavior of deletecolumn! and show #48

Closed
leostenzel opened this issue Sep 28, 2023 · 4 comments · Fixed by #49
Closed

Surprising behavior of deletecolumn! and show #48

leostenzel opened this issue Sep 28, 2023 · 4 comments · Fixed by #49

Comments

@leostenzel
Copy link

Hi & thanks for your package,

I was just trying out the example, and was a bit surprised that the final matrix shows

 [1, 1] = 2 
 [1, 2] = 3 
 [1, 3] = 4 
 [1, 4] = 1 

 [nothing, 1] = 3 
 [nothing, 6] = 5 
 [nothing, 8] = 7  

It appears that either deletecolumn! needs to delete the corresponding element of col_keys, or the tmp counter in show does not work correctly?

Slightly related: It was a bit confusing to me that show seems to exchange the order of indices. I.e., matrix[4,1] = 1 added an entry [1, 4] = 1

Lastly an easy fix, there's an s too much after throw:

col_key != col && throws(ArgumentError("column $(col) does not exist."))

@guimarqu
Copy link
Contributor

Hi thanks for the report,

Can you send me your example please?
I think the issue comes from show because it was implemented very quickly.

The order is indeed not correct.

@leostenzel
Copy link
Author

Ah, sorry, I just c&p the example from the github page:

using DynamicSparseArrays
I = [1, 2, 3, 2, 6, 7, 1, 6, 8] #rows
J = [1, 1, 1, 2, 2, 2, 3, 3, 3] #columns
V = [2, 3, 4, 2, 4, 5, 3, 5, 7] #value
matrix = dynamicsparse(I,J,V) # create a matrix

matrix[4,1] = 1 # new column
matrix[2,2] = 0 # delete value
deletecolumn!(matrix, 2) # delete column with id 2
matrix[2,6] == 0 # true

@guimarqu
Copy link
Contributor

Thanks for the report. That was a bug in show. I release a new version when the fix is merged into master. You can find a brief description of the project here: https://atoptima.github.io/Coluna.jl/stable/dynamic_sparse_arrays/

@leostenzel
Copy link
Author

Cool, that was quick :)
Thanks, I'll have a look!

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 a pull request may close this issue.

2 participants