Increasing Test coverage

[jeffreypaul15]

Description

Fixes #33
Tests have yet to be added.

• fit_polynomial_to_log_radial_intensity
• normalize_fit_radial_intensity
• calculate_fit_radial_intensity
• intensity_enhance
• calc_gamma
• remove_duplicate
• points_in_poly
• reform2d
• Lamb_Oseen

[pep8speaks]

Hello @jeffreypaul15! Thanks for updating this PR.

• In the file sunkit_image/tests/test_radial.py:

Line 242:101: E501 line too long (102 > 100 characters)
Line 258:101: E501 line too long (106 > 100 characters)
Line 262:101: E501 line too long (108 > 100 characters)
Line 293:101: E501 line too long (101 > 100 characters)

• In the file sunkit_image/utils/tests/test_utils.py:

Line 122:101: E501 line too long (102 > 100 characters)
Line 135:101: E501 line too long (107 > 100 characters)
Line 156:101: E501 line too long (102 > 100 characters)

Comment last updated at 2021-04-10 00:58:11 UTC

[jeffreypaul15]

@nabobalis Are these tests fine or should I have to manually store the lists and then check for assert np.allclose instead of calculating them and then checking?

Also, the following methods return enhanced SunPy maps, would figure tests be required for them?

sunkit-image/sunkit_image/radial.py

Line 107 in bd40d86

def intensity_enhance(

sunkit-image/sunkit_image/radial.py

Line 361 in bd40d86

def fnrgf(

sunkit-image/sunkit_image/radial.py

Line 209 in bd40d86

def nrgf(

[nabobalis]

The tests look fine, I assume the ipython notebook was just for your use?

Figure tests or normal unit test would be ok. The figure-tests is broken due to a deprecation warning, probably best to ignore it for now in setup.cfg

[jeffreypaul15]

The tests look fine, I assume the ipython notebook was just for your use?

Figure tests or normal unit test would be ok. The figure-tests is broken due to a deprecation warning, probably best to ignore it for now in setup.cfg

Oh yeah, sorry about the notebook I'll remove it

[jeffreypaul15]

@nabobalis methods from Lamb_Oseen class are already tested but a few are missing, I shall add them slowly.
Are the remaining tests written fine? They should pass the required tests now.

Also, should I go through with the figure tests? I mean, I have tested them with unit tests so I don't really think they're required.

[jeffreypaul15]

py37-oldestdeps [linux] is failing and I'm a bit confused as to why.
The part where it fails runs fine on py38.

[nabobalis]

What about this one?

[jeffreypaul15]

Oh yeah, I forgot about this. I'll add this.

[nabobalis]

py37-oldestdeps [linux] is failing and I'm a bit confused as to why.
The part where it fails runs fine on py38.

Well a package difference means the test output is different.

[jeffreypaul15]

py37-oldestdeps [linux] is failing and I'm a bit confused as to why.
The part where it fails runs fine on py38.

Well a package difference means the test output is different.

Yeah the output is different for both of the tests, is there a way I can get a different output for the different versions?

[nabobalis]

py37-oldestdeps [linux] is failing and I'm a bit confused as to why.
The part where it fails runs fine on py38.

Well a package difference means the test output is different.

Yeah the output is different for both of the tests, is there a way I can get a different output for the different versions?

Run the oldest-deps build locally or install the package versions that have changed in the oldest-deps build.

[jeffreypaul15]

Run the oldest-deps build locally or install the package versions that have changed in the oldest-deps build.

I've checked for the version of change in skimage as that seemed to be causing the change in output.

[nabobalis]

Run the oldest-deps build locally or install the package versions that have changed in the oldest-deps build.

I've checked for the version of change in skimage as that seemed to be causing the change in output.

It would be better to understand what is causing the change and then decide if we need to change the minimum package version.

[nabobalis]

Probably worth adding a changelog considering how many tests are here.

[jeffreypaul15]

It would be better to understand what is causing the change and then decide if we need to change the minimum package version.

I suppose, I shall dig a little deeper and try to find out.

Probably worth adding a changelog considering how many tests are here.

Would trivial do?

[nabobalis]

Would trivial do?

Works for me.

[jeffreypaul15]

After looking around, I've found that skimage's measure.grid_points_in_poly doesn't produce the right output before version 0.18. Based on the discussion at scikit-image/scikit-image#3892, it seems to have been fixed.

Ideally grid_points_in_poly takes in an input of a grid (ex: 3,3) with the edges of a polygon (Nx2) array and outputs a boolean array with the same shape as that of the grid, returning True for all the points on the grid that are part-of/ inside the given polygon.

# version < 0.18.0, produces buggy output
poly = [(0, 0), (0, 1), (0, 2), (1, 2), (2, 2), (2, 0)]
mask = measure.grid_points_in_poly((3,3), poly)
print(mask)
[[ True  True False]
 [ True  True False]
 [False False False]]

# version >= 0.18.0
poly = [(0, 0), (0, 1), (0, 2), (1, 2), (2, 2), (2, 0)]
mask = measure.grid_points_in_poly((3,3), poly)
print(mask)
array([[ True,  True,  True],
       [ True,  True,  True],
       [ True,  True,  True]])

In the above example the points

[(0,2), (1,2), (2,0), (2,1), (2,2)]

all lie on the edge of the polygon but based on previous versions of skimage produces False for those points.

This was buggy before and didn't return the right boolean array but it seemed to have been fixed in 0.18.
I suggest we change the default version because of this.

[nabobalis]

Thanks for looking in to it. Change the version in setup.cfg and bump the version in tox.ini for the oldest deps.

[jeffreypaul15]

Thanks for looking in to it. Change the version in setup.cfg and bump the version in tox.ini for the oldest deps.

Will do.

[nabobalis]

Thanks for looking in to it. Change the version in setup.cfg and bump the version in tox.ini for the oldest deps.

Will do.

I think this will want a seperate changelog as well.

[jeffreypaul15]

(Core Tests py38 [linux]) seems to be failing, is it because of the change in setup or tox.ini files?

[nabobalis]

Looks like some internet issues.

[jeffreypaul15]

@nabobalis I suppose all the tests are fine?

[nabobalis]

Yeah the tests are fine.

[jeffreypaul15]

Would we want to add Lamb_Oseen tests in this PR itself?

[nabobalis]

Would we want to add Lamb_Oseen tests in this PR itself?

Up to you.

[jeffreypaul15]

Up to you.

I'll add them here itself.

[jeffreypaul15]

This should cover all the required tests for now. If all the tests pass, would this be good to merge?

[nabobalis]

Thanks for the PR @jeffreypaul15