updated regionprops_table test for iterable properties

[yfukai]

Description

Associated with pull request #5239 and issue #5222 . I updated test test_regionprops_table_equal_to_original so that they compare not only non-iterable properties but also iterable ones of regionprops and regionprops_table.

Checklist

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

For reviewers

• 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.

[pep8speaks]

Hello @yfukai! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-02 06:44:10 UTC

[mkcor]

Excellent, thank you! With the breakdown introduced by separator="-", I'm not seeing any instance where i > 0... Please correct me if I'm missing something!
It seems to me that all properties are covered by this main for loop:

    for prop, dtype in COL_DTYPES.items():
        for reg in regions:
            rp = reg[prop]
            if np.isscalar(rp) or \
                    prop in OBJECT_COLUMNS or \
                    dtype is np.object_:
                assert_array_equal(rp, out_table[prop][0])
            else:
                shape = rp.shape if isinstance(rp, np.ndarray) else (len(rp),)
                for ind in np.ndindex(shape):
                    modified_prop = "-".join(map(str, (prop,) + ind))
                    loc = ind if len(ind) > 1 else ind[0]
                    assert_equal(rp[loc], out_table[modified_prop][0])

[yfukai]

Hi, thank you for your comment @mkcor ! Yeah you're right, SAMPLE has only one region with the label 1, so i>0 doesn't appear in the loop for now. However, I have to note that I think the modification you suggested would result in an error when someone add label 2 to SAMPLE (if I understand correctly). Instead we could fix the index to 0 as follows to make the code a bit simpler:

reg=regions[0]
for prop, dtype in COL_DTYPES.items():
    rp = reg[prop]
    if np.isscalar(rp) or \
            prop in OBJECT_COLUMNS or \
            dtype is np.object_:
        assert_array_equal(rp, out_table[prop][0])
    else:
        shape = rp.shape if isinstance(rp, np.ndarray) else (len(rp),)
        for ind in np.ndindex(shape):
            modified_prop = "-".join(map(str, (prop,) + ind)) 
            loc = ind if len(ind) > 1 else ind[0]
            assert_equal(rp[loc], out_table[modified_prop][0])

I'd be happy to hear your opinion on this!

[mkcor]

@yfukai ok, very well. This way your test is robust to the case where SAMPLE would have more than one region. Fair enough! Then I'm happy to approve the code as is.

[yfukai]

Wonderful, thanks!

[rfezzani]

Thank you @yfukai! The PR is really good. I just left two small suggestions 😉

[rfezzani]

Suggested change

	if np.isscalar(rp) or \
	prop in OBJECT_COLUMNS or \
	dtype is np.object_:
	if np.isscalar(rp) or prop in OBJECT_COLUMNS:

The dtype is np.object_ condition is not necessary.

[yfukai]

Oh thanks, I see! This is actually a copy from the logic in _regionprops.py itself. I'll try to rewrite it in a bit more DRY way.

[jni]

Awesome, thank you again @yfukai!

[yfukai]

Thanks for merging @jni , but I think I still haven't dealt with @rfezzani 's second comment. (Maybe I will make some new PR for that at some point! 🙏)

[jni]

Oops! 😅 I saw the comment and the commit and made an assumption. 😬 Future PRs are absolutely welcome! I think this is already a major improvement anyway. =)

[rfezzani]

😄 no worry, it is not a big deal! The PR is already in really good shape, but as @jni said, future PRs are welcome! Thank you again @yfukai 😉