Sparse test sets #18844
Sparse test sets #18844
Conversation
|
On line 127, line 325 is still testing line 333 is still testing line 488, leave the comment about sparse diff line 781 and 874 and 1062 and 1281 and 1302 and 1395, getindex and setindex and dropstored! and sparse/sparsevec and spdiagm and similar are not capitalized same on 1405, 1412, 1419, 1425, 1449, 1489, 1494, 1501, 1529, 1691 - don't capitalize function names line 1167, don't put a testset around a function definition line 1624, UniformScaling is the type name don't delete the comments that describe what #12177 and #12118 and #18705 mean |
|
Apologies, still learning the pull request process. I'm guessing I needed to have rebased my branch instead of merge committing. What is best way to handle this now that I have submitted pull request already? |
|
I misread since github unhelpfully won't show a diff this big. It might have been better if split up into multiple commits of a few hundred lines each, but that's a consequence of bad tools. Viewing the diff as https://github.com/JuliaLang/julia/pull/18844/files?w=1 works better but @github won't let you leave review comments on that view. The merge commit didn't hurt anything from what I can tell. You can address the review as additional commits to this branch in your fork, and we can squash upon merging. |
|
I have all the changes ready but for these two comments:
Do these mean that titles of test sets on these lines should mention these functions? |
|
they do on master (the comments anyway). you changed the titles so they don't any more, which wasn't necessary |
| Base.SparseArrays.getindex_I_sorted_linear(A, I, J) | ||
| end | ||
|
|
||
| @testset "test_get_index_algs" begin |
There was a problem hiding this comment.
the function is called test_getindex_algs
There was a problem hiding this comment.
I meant you should remove the underscore from get_index, not drop the test_ prefix
|
leave " for specialized matrix types" on line 1694 |
|
I pushed a minor revision to this branch. Note that lines 663-702 aren't related to issue #13024, and shouldn't be in the same testset. |
|
@tkelman is this merge-able? |
|
not until the "shouldn't be in the same testset" comment is addressed |
|
Sorry, I misunderstood the meaning of your comment and thought you fixed this in your revision. I will take a look at this. |
|
I am guessing I mistakenly assumed all sections were commented due to the excellent commenting in file. I didn't see comments in original for these blocks, so I attempted to name them myself. These are probably awful testset names, so feel free to correct. |
| @@ -738,6 +697,53 @@ end | |||
| end | |||
| end | |||
|
|
|||
| @testset "Test equivalency of created identity matrices from sparse matrix" begin | |||
There was a problem hiding this comment.
best to check git blame and see when this was introduced to find the actual intent, instead of completely guessing
There was a problem hiding this comment.
also best to put these back in the order in the file they were originally, so the diff is easier to review
| @@ -661,6 +661,53 @@ end | |||
| end | |||
| end | |||
|
|
|||
| @testset "spones" begin | |||
There was a problem hiding this comment.
this still isn't an "issue" - by original location I meant where they were in the file before your PR, not at an earlier commit
|
I moved them to after the first non issue related tests I could find in the original file where they took place. Sorry and thanks for patience. |
|
bump! this needs a rebase. |
|
@lbollar Would be great if you could rebase to master and we should get this merged asap. Thanks for the work and patience. |
|
Traveling, but will try and get this done tonight when I get back home to |
|
I am sorry all. I became very busy. The merge conflicts created over time are more difficult to solve than just using the latest commit and what I have here as a guideline to redo everything. Most of this pull request thread has been spent discussing naming and organization, which is mostly complete now, so the rewrite shouldn't that be painful. After discussing with @kshyatt, I think this is best route. Semester is ending so I should be able to rewrite soon, hopefully looking at doing it this weekend. Sorry for the delay. |
|
Thanks. Looking forward to having the new PR. Perhaps we should leave this one open in the meanwhile. |
Referencing #18801
Added testsets to all of sparse.jl test file. For tests clearly mentioning issues, I have tried to group them in an issues testset with each subset testset containing an individual issue.