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

[MRG] fix scaled=1 gather bug #1670

Merged
merged 1 commit into from
Jul 14, 2021
Merged

[MRG] fix scaled=1 gather bug #1670

merged 1 commit into from
Jul 14, 2021

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Jul 14, 2021

Creates a test for & and fixes a bug noted in #1421 (comment) - the default "unset" value used in the #1613 code was set to a valid scaled value, and we never tested it! Oops!

@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #1670 (d101fb3) into latest (02f2128) will increase coverage by 7.40%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1670      +/-   ##
==========================================
+ Coverage   82.35%   89.76%   +7.40%     
==========================================
  Files         113       86      -27     
  Lines       11772     8058    -3714     
  Branches     1490     1490              
==========================================
- Hits         9695     7233    -2462     
+ Misses       1818      566    -1252     
  Partials      259      259              
Flag Coverage Δ
python 89.76% <100.00%> (ø)
rust ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sourmash/search.py 96.91% <100.00%> (ø)
src/core/src/signature.rs
src/core/src/index/linear.rs
src/core/src/ffi/cmd/compute.rs
src/core/src/sketch/nodegraph.rs
src/core/src/ffi/mod.rs
src/core/src/index/sbt/mod.rs
src/core/src/index/mod.rs
src/core/src/ffi/nodegraph.rs
src/core/src/ffi/hyperloglog.rs
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02f2128...d101fb3. Read the comment docs.

@ctb
Copy link
Contributor Author

ctb commented Jul 14, 2021

Ready for review & merge @sourmash-bio/devs

@taylorreiter
Copy link
Contributor

the test LGTM, but I don't understand the difference between

self.cmp_scaled = 1

and

self.cmp_scaled = 0     # initialize with something very low!

@ctb
Copy link
Contributor Author

ctb commented Jul 14, 2021

the test LGTM, but I don't understand the difference between

self.cmp_scaled = 1

and

self.cmp_scaled = 0     # initialize with something very low!

The issue is this line

if self.cmp_scaled != max_scaled:
, where we have an if statement that updates the object properties if and only if scaled has increased. I used this to initialize the object properties the first time through by setting the initial cmp_scaled to a value of 1, which means (before this change) it would only initialize the object properties for a scaled > 1.

It's not great code, to be fair. But rather than making bigger changes I decided to just patch it and test it :)

@taylorreiter
Copy link
Contributor

where we have an if statement that updates the object properties if and only if scaled has increased. I used this to initialize the object properties the first time through by setting the initial cmp_scaled to a value of 1, which means (before this change) it would only initialize the object properties for a scaled > 1

Ah this makes sense, thank you for the explanation!

Copy link
Contributor

@taylorreiter taylorreiter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ctb ctb merged commit 0393456 into latest Jul 14, 2021
@ctb ctb deleted the fix/scaled_1_gather branch July 14, 2021 15:54
@ctb
Copy link
Contributor Author

ctb commented Jul 14, 2021

thanks!

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.

2 participants