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

fix findmin/findmax #93

Merged
merged 9 commits into from
Apr 26, 2022
Merged

fix findmin/findmax #93

merged 9 commits into from
Apr 26, 2022

Conversation

Moelf
Copy link
Contributor

@Moelf Moelf commented Apr 22, 2022

@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2022

Codecov Report

Merging #93 (6e80751) into main (f8ffbb7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #93   +/-   ##
=======================================
  Coverage   89.18%   89.18%           
=======================================
  Files           7        7           
  Lines        5677     5679    +2     
=======================================
+ Hits         5063     5065    +2     
  Misses        614      614           
Impacted Files Coverage Δ
src/sparsevector.jl 94.17% <100.00%> (+<0.01%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Moelf
Copy link
Contributor Author

Moelf commented Apr 22, 2022

Error seems unrelated

src/sparsevector.jl Outdated Show resolved Hide resolved
@Moelf Moelf requested a review from KristofferC April 22, 2022 16:43
@Moelf
Copy link
Contributor Author

Moelf commented Apr 22, 2022

404 error is unrelated

src/sparsevector.jl Outdated Show resolved Hide resolved
@Moelf
Copy link
Contributor Author

Moelf commented Apr 22, 2022

looks like that logic is flawed

@Moelf Moelf force-pushed the main branch 2 times, most recently from cf062ab to 9cf449e Compare April 22, 2022 18:48
src/sparsevector.jl Outdated Show resolved Hide resolved
Co-authored-by: Steven G. Johnson <[email protected]>
@KristofferC
Copy link
Member

@dkarrasch, I didn't realize you did not have merge access here. I think you should have gotten an invite now.

@dkarrasch
Copy link
Member

@KristofferC I think I already had merge access, I was just a bit hesitant to push the button. Tests pass, which is good, but I haven't completely understood the originally missing case here.

@KristofferC
Copy link
Member

I think I already had merge access

Your approve symbol was gray which I thought typically means that the person reviewing does not have merge rights. It is green now, however.

@Moelf
Copy link
Contributor Author

Moelf commented Apr 26, 2022

The missing case is when the sparse vector is filled with value from the beginning but not all the way to the end.

The original "find first non-stored zero" rely on there existing a gap in the stored values

@dkarrasch dkarrasch merged commit 352dfaf into JuliaSparse:main Apr 26, 2022
@dkarrasch
Copy link
Member

Thanks for the fix! Now, how do we make sure this enters julia v1.8?

@Moelf
Copy link
Contributor Author

Moelf commented Apr 26, 2022

@dkarrasch
Copy link
Member

Alright, so the bot will jump in at some point.

@Moelf
Copy link
Contributor Author

Moelf commented Apr 26, 2022

idk if the backport to 1.8 will have to be manual though @DilumAluthge

@oscardssmith
Copy link

did we ever back port this?

@Moelf
Copy link
Contributor Author

Moelf commented Jun 22, 2022

@DilumAluthge

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

The backport to 1.8 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.8 1.8
# Navigate to the new working tree
cd .worktrees/backport-1.8
# Create a new branch
git switch --create backport-93-to-1.8
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 352dfafd4a31782c316c6aac69806336b979922c
# Push it to GitHub
git push --set-upstream origin backport-93-to-1.8
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.8

Then, create a pull request where the base branch is 1.8 and the compare/head branch is backport-93-to-1.8.

dkarrasch pushed a commit that referenced this pull request Sep 2, 2022
Co-authored-by: Steven G. Johnson <[email protected]>
dkarrasch pushed a commit that referenced this pull request Sep 2, 2022
Co-authored-by: Steven G. Johnson <[email protected]>
rayegun pushed a commit that referenced this pull request Sep 8, 2022
Co-authored-by: Steven G. Johnson <[email protected]>
@rayegun rayegun mentioned this pull request Sep 8, 2022
4 tasks
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.

argmin / findmin incorrect behaviour on SparseVector on 1.8
6 participants