Please have a look at bytescale tests?

[lwasser]

hey @joemcglinchy
can you please have a look at this here

https://travis-ci.org/earthlab/earthpy/builds/460164320#L1240

it's throwing an error on @betatim pr
what is odd about this is that the last commit actually did not fail. but in this pr clearly the bytscale tests are throwing an error.

Can you kindly look into this as this is related to that pr that got merged accidentally?
Many thanks!

[joemcglinchy]

@lwasser this means that bytescale is not returning an array where the minimum value is 0.0, rather, it is 1.0.... why would that be?

[lwasser]

i'm not sure. So can you run the test_spatial locally and see if it throws an error and explore it perhaps?
Looking at the code below, it looks like the tests aren't quite complete so a bit of cleanup is also in order :)

i would imagine tho if you run it locally it will throw the same assert as travis does? does it? and if not ... i am not sure. does the cmin have anything to do with it perhaps?

# Test scale value max is less equal to min
    es.bytescale(arr, cmin=100, cmax=100)
    # assert something    
        
    # Test scale value max is less equal to min
    scale_arr = es.bytescale(arr, cmin=10, cmax=240)
    assert scale_arr.min() == 0
    assert scale_arr.max() == 255

[joemcglinchy]

There is nearly 100% code coverage on the bytescale function, with the exception of checking if the data is of type uint8, which would be easy enough.
https://codecov.io/gh/earthlab/earthpy/src/master/earthpy/spatial.py#L259

I am testing it locally and am receiving no errors when calling test_bytescale_high_low_val(). are you seeing errors with it locally? the cmin parameter should have nothing to do with it. cmin and cmax essentially define the data range for which to convert to [low, high], which are defined as default to be [0, 255], so that assert statement should not ever evaluate to 1==0

[lwasser]

can you do this? @joemcglinchy can you please submit a new pr... in that just clean up the file - specifically remove this:

    # Test scale value max is less equal to min
    es.bytescale(arr, cmin=100, cmax=100)
    # assert something   

the code above doesn't actually do anything. let's see what travis says on a new build? when i run it locally i don't see an error. the min is 0 . i'm not sure what travis is running.

[joemcglinchy]

removing that line will remove coverage for the elif scale == 0: line in bytescale:

    if cscale < 0:
        raise ValueError("`cmax` should be larger than `cmin`.")
    elif cscale == 0:
        cscale = 1

[lwasser]

ok @joemcglinchy since things seem to be ok with travis now i'll close this.if travis becomes angry again we can revisit! thank you so much.

[betatim]

Could it be that this error only shows up for some unlucky values of the array because the values are generated randomly (I think)?

[lwasser]

@betatim it could be the case! i'd have to think about this more in context of the function. pretty odd that it worked after a travis build so that does suggest your statement must be true.

[joemcglinchy]

it certainly could be the case. I ran that function in a loop with 1000000 iterations, multiple times, and that assertion error comes up randomly.

I brought this up to Leah yesterday, I think there is something else that could be contributing to this issue. The return statement of bytescale ensures that if there is a value of 0 anywhere in the array, it automatically assumes the value of 1 due to the +0.5 and conversion to uint8 which applies a floor to the data values. If we update the doc to describe that, and change the assert statement, then that would be fine. However, due to the random init of that array in the test function we could still see failures there.

[joemcglinchy]

looks like we have a bug in bytescale. I edited my local copy of test_spatial.py to print out some values, here's what we get for the last check on scale_arr which is throwing the error

iteration 71
min of 4 --> 0
max of 299 --> 255

iteration 72
min of 5 --> 0
max of 299 --> 255

iteration 73
min of 4 --> 0
max of 293 --> 255

iteration 74
min of 10 --> 0
max of 299 --> 255

iteration 75
min of 3 --> 0
max of 294 --> 255

iteration 76
min of 0 --> 0
max of 295 --> 255

iteration 77
min of 1 --> 0
max of 298 --> 255

iteration 78
min of 0 --> 0
max of 299 --> 255

iteration 79
min of 0 --> 0
max of 299 --> 255

iteration 80
min of 0 --> 0
max of 295 --> 255

iteration 81
min of 3 --> 0
max of 299 --> 255

********** HERE IT IS ************
iteration 82
min of 14 --> 4
max of 293 --> 255
**********************************

Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "C:\....\earthpy\earthpy\tests\test_spatial.py", line 90, in test_bytescale_high_low_val
    assert scale_arr.min() == 0
AssertionError

[lwasser]

what about that particular array threw the bug @joemcglinchy ? Nice find!!

[joemcglinchy]

I think since the min of the original array was greater than the cmin specified (10)? I am not totally sure. I'm going to have to figure that one out locally. I'll add it to my bytescale_tests branch and submit another PR when it's ready.

[lwasser]

Sounds great @joemcglinchy you know i've found a bunch of bugs in my functions too working through these unit tests. i think for that reason alone it's a great option. :)