HOG returns incorrect features if pixels_per_cell is odd

[t-ae]

Description

skimage.feature.hog was ignoring last row/column of cell if pixels_per_cell is odd number.

Suppose pixels_per_cell == (3, 3)

In here cell_rows == pixels_per_cell[0].

scikit-image/skimage/feature/_hoghistogram.pyx

Lines 120 to 127 in 9c4632f

	r_0 = cell_rows / 2
	c_0 = cell_columns / 2
	cc = cell_rows * number_of_cells_rows
	cr = cell_columns * number_of_cells_columns
	range_rows_stop = cell_rows / 2
	range_rows_start = -range_rows_stop
	range_columns_stop = cell_columns / 2
	range_columns_start = -range_columns_stop

range_rows_stop = cell_rows / 2 = 1
range_rows_start = -range_rows_stop = -1

cell_rows is 3 but [range_row_start, range_rows_stop) contains only 2 integers, so last row will be ignored.
I fixed range_rows_stop (and range_columns_stop) calculation.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

• Clean style in the spirit of PEP8
• Docstrings for all functions
• Gallery example in ./doc/examples (new features only)
• Benchmark in ./benchmarks, if your changes aren't covered by an
  existing benchmark
• Unit tests

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

• Check that the PR title is short, concise, and will make sense 1 year
  later.
• Check that new functions are imported in corresponding __init__.py.
• Check that new features, API changes, and deprecations are mentioned in
  doc/release/release_dev.rst.
• Consider backporting the PR with @meeseeksdev backport to v0.14.x

[t-ae]

By the way I wanna add test code.
But existing test is using prepared npy file.
How should I get the correct features?

[emmanuelle]

Thanks for noticing this bug. The change seems to be correct. For adding tests, maybe you could use a very small image (like a = np.ones((9, 9)); a[-1] = 1; feature.hog(a, pixels_per_cell=(3, 3)) and check that the result is different with your change (ie, the last line is taken into account).

[pep8speaks]

Hello @t-ae! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 17, 2019 at 01:59 Hours UTC

[t-ae]

The bug is that last row/column are skipped while scanning pixels.
To verify the problem, I added a test case about the image below.

[[0, 0, 0],
 [0, 0, 0],
 [0, 0, 1]]

The output feature will be non-zero if last row/column are not skipped.
(non-zero elements should be 0.5 but they appear as 0.4999775, maybe because eps is added while L1 normalization.)

Now it's ready to be merged.

[emmanuelle]

Great, I love this new test. Can you just remove all keyword arguments which have default values (orientations, etc.) and are therefore not needed, in order to keep a small codebase even in tests? @scikit-image/core this one is almost ready!

[t-ae]

Removed them :)

[Borda]

good work :)

[grlee77]

The fix looks correct and is covered by a new test. Thanks @t-ae

The CI failure seems unrelated