Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacing data/camera.png due to copyright #4913

Merged
merged 78 commits into from
Oct 2, 2020

Conversation

alexdesiqueira
Copy link
Member

Description

Removing data/camera.png (a.k.a. cameraman) due to copyright issues. Solves #3927.

@grlee77
Copy link
Contributor

grlee77 commented Aug 13, 2020

This image is pretty widely used in lot of test cases, including some with some pre-computed results such as:

"restoration/tests/camera_rl.npy": "d219834415dc7094580abd975abb28bc7a6fb5ab83366e92c61ccffa66ca54fd",
"restoration/tests/camera_unsup.npy": "6d911fd0028ee78add8c416553097f15c6c4e59723ea32bd828f71269b6ea240",
"restoration/tests/camera_unsup2.npy": "30e81718f3cac0fc00d84518ca75a3c0fb9b246bb7748a9e20ec0b44da33734d",
"restoration/tests/camera_wiener.npy": "71e7cab739d6d145a288ec85dd235a62ff34442ccd1488b08139bc809850772b",

I am wondering if we can keep using it in the test suite for that purpose, but just not keep it as part of the public API or demos in the documentation? Wouldn't that usage fit within the non-profit restrictions of the license?

@pep8speaks
Copy link

pep8speaks commented Aug 13, 2020

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

Line 68:80: E501 line too long (90 > 79 characters)
Line 121:80: E501 line too long (106 > 79 characters)
Line 122:80: E501 line too long (109 > 79 characters)
Line 123:80: E501 line too long (110 > 79 characters)
Line 124:80: E501 line too long (110 > 79 characters)

Line 26:80: E501 line too long (80 > 79 characters)
Line 82:21: E241 multiple spaces after ','
Line 82:28: E241 multiple spaces after ','
Line 82:35: E241 multiple spaces after ','
Line 83:35: E241 multiple spaces after ','
Line 84:35: E241 multiple spaces after ','
Line 85:28: E241 multiple spaces after ','
Line 85:35: E241 multiple spaces after ','
Line 89:26: E128 continuation line under-indented for visual indent

Line 64:57: E261 at least two spaces before inline comment

Comment last updated at 2020-09-30 12:15:11 UTC

@alexdesiqueira
Copy link
Member Author

@grlee77 indeed, I'm seeing that now! 😅

I am wondering if we can keep using it in the test suite for that purpose, but just not keep it as part of the public API or demos in the documentation?

Personally, I'd not be comfortable with that, Greg. It would give us some work (by Tupã, I know 😂), but IMHO it's not like we don't have alternatives around with less restrictive licenses. Of course, my two BRL cents.

@grlee77
Copy link
Contributor

grlee77 commented Aug 13, 2020

Personally, I'd not be comfortable with that, Greg. It would give us some work (by Tupã, I know joy), but IMHO it's not like we don't have alternatives around with less restrictive licenses. Of course, my two BRL cents.

Sure, in most cases it is probably trivial to switch. For the few with precomputed outputs, I guess we can just regenerate those for a different image and update the registry.

@alexdesiqueira
Copy link
Member Author

alexdesiqueira commented Aug 13, 2020

@grlee77 yeah, I was thinking about that. @stefanv wanted a nice image to take cameraman's place, but we didn't get it yet... I'll search for some viable candidates today. I put grass in its place just for now, but we'll have to recalculate everything for the new image.

@stefanv
Copy link
Member

stefanv commented Aug 14, 2020

Best to just bite the bullet, I think. It wouldn't be the end of the world to use it internally, but it is shouldn't be too hard to switch.

@alexdesiqueira alexdesiqueira changed the title Removing data/camera.png due to copyright Replacing data/camera.png due to copyright Aug 16, 2020
@alexdesiqueira
Copy link
Member Author

Alright, based on the discussion we had on #4914, data.camera will point to a new camera.png.

@emmanuelle
Copy link
Member

I was surprised that all tests passed with the new image but there is a catch: pooch uses the data files in master because of https://github.com/scikit-image/scikit-image/blob/master/skimage/data/__init__.py#L120, so that the old image is used (as you can see from https://225-2014929-gh.circle-artifacts.com/0/doc/build/html/auto_examples/data/plot_general.html#sphx-glr-auto-examples-data-plot-general-py for example).

As a workaround, could you please modify https://github.com/scikit-image/scikit-image/blob/master/skimage/data/__init__.py#L120 so that it points to your repo and branch? Then we can see if tests pass and the doc looks good, and of course we'll revert the code in data before merging.

@alexdesiqueira
Copy link
Member Author

Sure thing @emmanuelle, thanks for the catch! You're right, I think it's very unlikely the tests will pass for the new image 🙂

@alexdesiqueira alexdesiqueira added 🔧 type: Maintenance Refactoring and maintenance of internals 💪 Work in progress labels Aug 18, 2020
@alexdesiqueira alexdesiqueira changed the title Replacing data/camera.png due to copyright [WIP] Replacing data/camera.png due to copyright Aug 18, 2020
@emmanuelle
Copy link
Member

I pushed a commit to your branch because the URL where to find the files was not the correct one I think, let's keep fingers crossed this one works!

@emmanuelle
Copy link
Member

ok, you also need to modify the hash in the registry (see the error in CI).

@emmanuelle
Copy link
Member

We're making progress, now the next thing is to fix the 25 failing tests...

FAILED exposure/tests/test_exposure.py::test_equalize_masked - assert 1.23262...
FAILED feature/tests/test_canny.py::TestCanny::test_use_quantiles - Assertion...
FAILED filters/tests/test_ridges.py::test_2d_cropped_camera_image - ValueErro...
FAILED filters/tests/test_ridges.py::test_3d_cropped_camera_image - ValueErro...
FAILED filters/tests/test_thresholding.py::test_otsu_camera_image - assert 91...
FAILED filters/tests/test_thresholding.py::test_li_camera_image - assert 69.7...
FAILED filters/tests/test_thresholding.py::test_yen_camera_image - assert 199...
FAILED filters/tests/test_thresholding.py::test_isodata_camera_image - assert...
FAILED filters/tests/test_thresholding.py::test_threshold_minimum - Assertion...
FAILED io/tests/test_collection.py::TestImageCollection::test_concatenate - V...
FAILED io/tests/test_io.py::test_imread_file_url - assert (245, 327) == (512,...
FAILED measure/tests/test_structural_similarity.py::test_gaussian_mssim_vs_IPOL
FAILED measure/tests/test_structural_similarity.py::test_gaussian_mssim_vs_author_ref
FAILED measure/tests/test_structural_similarity.py::test_gaussian_mssim_and_gradient_vs_Matlab
FAILED measure/tests/test_structural_similarity.py::test_mssim_vs_legacy - As...
FAILED metrics/tests/test_simple_metrics.py::test_PSNR_vs_IPOL - AssertionErr...
FAILED metrics/tests/test_structural_similarity.py::test_gaussian_structural_similarity_vs_IPOL
FAILED metrics/tests/test_structural_similarity.py::test_gaussian_mssim_vs_author_ref
FAILED metrics/tests/test_structural_similarity.py::test_gaussian_mssim_and_gradient_vs_Matlab
FAILED metrics/tests/test_structural_similarity.py::test_mssim_vs_legacy - As...
FAILED restoration/tests/test_restoration.py::test_wiener - AssertionError: 
FAILED restoration/tests/test_restoration.py::test_unsupervised_wiener - Asse...
FAILED restoration/tests/test_restoration.py::test_richardson_lucy - Assertio...
FAILED util/tests/test_random_noise.py::test_salt_and_pepper - assert (3052 /...
FAILED util/tests/test_random_noise.py::test_clip_speckle - assert (1.2706687...

Note that the tests probably fail on your machine too, it will be easier to fix them and run the tests locally.

@alexdesiqueira
Copy link
Member Author

I'm on it 🙂 It seems we used cameraman too much... 😂

@alexdesiqueira
Copy link
Member Author

Kindly pinging @grlee77 🙂 Could you help us with the coefficients for the latest image? Thanks!

@grlee77
Copy link
Contributor

grlee77 commented Sep 29, 2020

Kindly pinging @grlee77 slightly_smiling_face Could you help us with the coefficients for the latest image? Thanks!

Here you go!

IPOL results
SSIM: 0.357959091663361
PSNR: 22.409353363576034

@alexdesiqueira
Copy link
Member Author

All green again 🎉 Gotta point the first reviewers one more time (@emmanuelle @rfezzani @jni). Thanks again @grlee77 and @JDWarner for all your help!

@jni
Copy link
Member

jni commented Sep 30, 2020

Whoo! I updated plot_glcm with the new sky and grass locations. The other example looks ok, not great, but master also looks ok, not great, so I say leave it for now. My approval from before still stands. Thank you everyone who has contributed to this (a lot of people now! 😅) and particularly @alexdesiqueira for the hard work here! I'm really happy with the end result.

@jni
Copy link
Member

jni commented Sep 30, 2020

@scikit-image/core the travis failure is unrelated and should not hold up a merge. =)

@sciunto
Copy link
Member

sciunto commented Sep 30, 2020

there is one line to be changed before merging.

Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @alexdesiqueira for your hard work on this 👏. Since the CI is green now, I approve this PR, but I think that we need to improve some gallery examples involving data.camera. Some examples where tuned for the old image. To illustrate this, please see:

  • Multi-Otsu Thresholding: new vs old
  • Markers for watershed transform: new vs old
  • Flood Fill: new vs old
  • Rank filters: new vs old

But this can be addressed in different PRs 😉

@sciunto
Copy link
Member

sciunto commented Sep 30, 2020

But this can be addressed in different PRs

+100

@alexdesiqueira
Copy link
Member Author

alexdesiqueira commented Sep 30, 2020

But this can be addressed in different PRs 😉

Sure thing :) I will open an issue to deal with them later; my repobranch is breaking already, I'd need a rebase... 😂

@sciunto
Copy link
Member

sciunto commented Oct 1, 2020

looks like there are some issues with checksums on azure, but not on travis...

@alexdesiqueira
Copy link
Member Author

looks like there are some issues with checksums on azure, but not on travis...

@sciunto the errors are due to last commit, since we don't have that image on base yet (please see 3f24084).

@emmanuelle
Copy link
Member

Yes a8cc0a9 was green.

@sciunto
Copy link
Member

sciunto commented Oct 2, 2020

Of course... I'm stupid.

@sciunto sciunto merged commit 8eadc24 into scikit-image:master Oct 2, 2020
@alexdesiqueira
Copy link
Member Author

Thanks everyone, we made it! 🎉

@hmaarrfk
Copy link
Member

hmaarrfk commented Oct 2, 2020

awesome job!

@stefanv
Copy link
Member

stefanv commented Oct 2, 2020

Thank you very much, @alexdesiqueira !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔽 Deprecation Involves deprecation 💪 Work in progress ⚠️ Priority This issue or PR should be given priority 🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.