[MRG] Doing bounds checking on --scaled via command line #1650

keyabarve · 2021-07-02T05:21:00Z

Fixes #1567

Add blocks of code in the functions from src/sourmash/commands.py one by one to check the bounds of --scaled.
Add tests for the blocks of code in each function one by one.
Concluded that the value of --scaled can be 0, which is the default value and hence it does not raise any errors.

keyabarve · 2021-07-02T05:22:06Z

@ctb I have created a new branch and PR and restarted working on issue #1567

mr-eyes · 2021-07-02T05:23:53Z

You mean #1243 ?

keyabarve · 2021-07-02T05:25:19Z

Yes, this ultimately fixes #1243, but the main goal is to find the issues with my code in #1567 and fix them.

codecov · 2021-07-02T05:27:18Z

Codecov Report

Merging #1650 (32254fe) into latest (67aba79) will increase coverage by 7.53%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           latest    #1650      +/-   ##
==========================================
+ Coverage   82.58%   90.11%   +7.53%     
==========================================
  Files         113       86      -27     
  Lines       11887     8188    -3699     
  Branches     1508     1511       +3     
==========================================
- Hits         9817     7379    -2438     
+ Misses       1813      555    -1258     
+ Partials      257      254       -3

Flag	Coverage Δ
python	`90.11% <100.00%> (+0.12%)`	⬆️
rust	`?`

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

Impacted Files	Coverage Δ
src/sourmash/commands.py	`87.37% <ø> (+1.13%)`	⬆️
src/sourmash/cli/gather.py	`100.00% <100.00%> (ø)`
src/sourmash/cli/index.py	`100.00% <100.00%> (ø)`
src/sourmash/cli/multigather.py	`100.00% <100.00%> (ø)`
src/sourmash/cli/prefetch.py	`100.00% <100.00%> (ø)`
src/sourmash/cli/search.py	`100.00% <100.00%> (ø)`
src/sourmash/cli/utils.py	`100.00% <100.00%> (ø)`
src/core/src/ffi/mod.rs
src/core/src/sketch/hyperloglog/estimators.rs
src/core/src/errors.rs
... and 25 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 67aba79...32254fe. Read the comment docs.

keyabarve · 2021-07-03T01:16:20Z

@ctb One of the tests over here is failing and I'm not sure why, because when I run py.test tests/test_sourmash.py on my computer, all the tests are passing.

keyabarve · 2021-07-03T05:26:56Z

When I added this chunk of code in all of the functions it worked.

if isinstance(args.scaled, float):
        if args.scaled:
            # add the necessary lines that were here earlier
            if args.scaled < 0:
                error('ERROR: --scaled value must be positive')
                sys.exit(-1)
            if args.scaled < 100:
                notify('WARNING: --scaled value should be >= 100. Continuing anyway.')
            if args.scaled > 1e6:
                notify('WARNING: --scaled value should be <= 1e6. Continuing anyway.')
        if args.scaled == 0:
            error('ERROR: --scaled value must be >= 1')
            sys.exit(-1)

This passed all the tests, except when I added this chunk of code on line 408 of src/sourmash/commands.py. When I replaced the current code on that line with this chunk, it started to fail the tests.

keyabarve · 2021-07-03T05:44:30Z

The latest commit contains the code that works on line 413 of src/sourmash/commands.py and passes all the tests. The code that doesn't work is commented out and is on lines 412 and 422-424.

ctb · 2021-07-05T22:14:05Z

hi @keyabarve, do you still want me to take a look at something? thx.

keyabarve · 2021-07-05T22:14:53Z

Yes, I'm not sure why one of the tests here on GitHub is failing still.

keyabarve · 2021-07-05T22:15:47Z

The latest commit contains the code that works on line 413 of src/sourmash/commands.py and passes all the tests. The code that doesn't work is commented out and is on lines 412 and 422-424.

With regards to this, could you please review the code and see if I should change it in any way or let it be as it is?

ctb · 2021-07-05T22:15:54Z

(all of the tests are passing on this PR)

keyabarve · 2021-07-05T22:18:34Z

Oh I meant checks! Sorry! codecov/patch — 77.50% of diff hit (target 81.91%) this one is failing and I'm not sure why...

ctb · 2021-07-05T22:19:51Z

The latest commit contains the code that works on line 413 of src/sourmash/commands.py and passes all the tests. The code that doesn't work is commented out and is on lines 412 and 422-424.

With regards to this, could you please review the code and see if I should change it in any way or let it be as it is?

sure - but I mostly have questions.

why are you checking to see if args.scaled is a float?

Two more specific questions -

In a previous comment (here) you found that it is an int in this case. Is that an error? Why is it an int there? Is it still an int here?
If args.scaled is always supposed to be a float, why are you checking to see if is a float here?

and, last but not least,

what is the line of code that, if it is not executed, leads to the tests failing?

ctb · 2021-07-05T22:27:39Z

Oh I meant checks! Sorry! codecov/patch — 77.50% of diff hit (target 81.91%) this one is failing and I'm not sure why...

It means that your new code isn't fully covered by tests, so at some point you'll need to add tests.

keyabarve · 2021-07-05T22:30:04Z

The latest commit contains the code that works on line 413 of src/sourmash/commands.py and passes all the tests. The code that doesn't work is commented out and is on lines 412 and 422-424.

With regards to this, could you please review the code and see if I should change it in any way or let it be as it is?

sure - but I mostly have questions.

why are you checking to see if args.scaled is a float?

This is mainly because I thought that the value of args.scaled is always supposed to be a float, but in some cases it is an int and I'm not sure why. I can completely remove this though.

Two more specific questions -

In a previous comment (here) you found that it is an int in this case. Is that an error? Why is it an int there? Is it still an int here?

I still haven't been able to figure out why it tends to be an int in some cases.

If args.scaled is always supposed to be a float, why are you checking to see if is a float here?

This is again, because it is an int sometimes and I'm not sure why.

and, last but not least,

what is the line of code that, if it is not executed, leads to the tests failing?

So it is not just one, but two lines of code:

if isinstance(args.scaled, float): on line 412

if args.scaled == 0:
           error('ERROR: --scaled value must be >= 1')
           sys.exit(-1)

on lines 422-424

keyabarve · 2021-07-05T22:30:55Z

Oh I meant checks! Sorry! codecov/patch — 77.50% of diff hit (target 81.91%) this one is failing and I'm not sure why...

It means that your new code isn't fully covered by tests, so at some point you'll need to add tests.

Hmm, I'm not sure what that means...

ctb · 2021-07-05T22:36:02Z

Oh I meant checks! Sorry! codecov/patch — 77.50% of diff hit (target 81.91%) this one is failing and I'm not sure why...

It means that your new code isn't fully covered by tests, so at some point you'll need to add tests.

Hmm, I'm not sure what that means...

See 'line coverage'. If you look at the "files changed" view for this PR here you'll see "Added line XYZ was not covered by tests", and what that means is that there are not tests that execute that line of code, which means it is completely untested.

ctb · 2021-07-05T22:39:35Z

The latest commit contains the code that works on line 413 of src/sourmash/commands.py and passes all the tests. The code that doesn't work is commented out and is on lines 412 and 422-424.

With regards to this, could you please review the code and see if I should change it in any way or let it be as it is?

sure - but I mostly have questions.
why are you checking to see if args.scaled is a float?

This is mainly because I thought that the value of args.scaled is always supposed to be a float, but in some cases it is an int and I'm not sure why. I can completely remove this though.

Two more specific questions -

In a previous comment (here) you found that it is an int in this case. Is that an error? Why is it an int there? Is it still an int here?

I still haven't been able to figure out why it tends to be an int in some cases.

If args.scaled is always supposed to be a float, why are you checking to see if is a float here?

This is again, because it is an int sometimes and I'm not sure why.

Please track down and understand why this is happening and explain it to me before asking what to do. As a reminder, you can use any or all of: the Python debugger, putting in print statements, or reading the code, to see how code behaves.

and, last but not least,

what is the line of code that, if it is not executed, leads to the tests failing?

So it is not just one, but two lines of code:
* `if isinstance(args.scaled, float):` on line 412
if args.scaled == 0:
           error('ERROR: --scaled value must be >= 1')
           sys.exit(-1)
on lines 422-424

ok, but neither of those lines of code changes anything. Are there lines of code in there that you didn't write that are now no longer getting executed due to the new if statements?

keyabarve · 2021-07-05T22:43:14Z

Please track down and understand why this is happening and explain it to me before asking what to do. As a reminder, you can use any or all of: the Python debugger, putting in print statements, or reading the code, to see how code behaves.

Okay, I'll try doing that.

So it is not just one, but two lines of code:
* `if isinstance(args.scaled, float):` on line 412
if args.scaled == 0:
           error('ERROR: --scaled value must be >= 1')
           sys.exit(-1)
on lines 422-424
ok, but neither of those lines of code changes anything. Are there lines of code in there that you didn't write that are now no longer getting executed due to the new if statements?

So, these blocks of code above have been commented out for now. But the rest of the code that is there from lines 413-421 is working and passing all tests.

keyabarve · 2021-07-15T03:00:10Z

@ctb Just to clarify, should I first try to look for possible errors in my own code, or should I change the tests that I have not written and are failing?

ctb · 2021-07-15T05:30:21Z

On Wed, Jul 14, 2021 at 08:00:22PM -0700, Keya Barve wrote: @ctb Just to clarify, should I first try to look for possible errors in my own code, or should I change the tests that I have not written and are failing?

Either way! My guess is that fixing the tests will help with finding possible errors in your own code.

keyabarve · 2021-07-15T21:26:06Z

@ctb Please review.

src/sourmash/cli/search.py

src/sourmash/cli/utils.py

src/sourmash/commands.py

ctb

A few minor requests for changes, but overall looking good!

…to KB_1243_copy

keyabarve · 2021-07-16T03:01:46Z

@ctb Please review.

src/sourmash/cli/utils.py

tests/test_sourmash.py

ctb

almost done!

keyabarve · 2021-07-16T18:55:27Z

@ctb Please review.

keyabarve · 2021-07-16T19:58:30Z

@ctb Please review.

ctb

🎉

Index function

9574fd5

keyabarve added 3 commits July 1, 2021 22:40

Search function

188788f

Gather function

db18b6f

Multigather function

8418195

Prefetch function

67d3b36

For loop Index function

6bcd26d

keyabarve added 5 commits July 2, 2021 23:13

Modified all functions

f149300

Search function tests

1609808

Gather function tests

53ce033

Multigather function tests

9d1c89e

Index function tests

e055984

keyabarve added 2 commits July 15, 2021 14:03

Made final changes

741af4b

Merge branch 'latest' into KB_1243_copy

d451db7