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

Add data files for new illuminants. #5276

Merged
merged 4 commits into from
Mar 30, 2021

Conversation

mkcor
Copy link
Member

@mkcor mkcor commented Mar 18, 2021

Description

Following up with #5234 -- only adding the new data here.

Checklist

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
Copy link

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

Line 44:80: E501 line too long (110 > 79 characters)
Line 46:80: E501 line too long (109 > 79 characters)
Line 47:80: E501 line too long (110 > 79 characters)
Line 48:80: E501 line too long (109 > 79 characters)
Line 49:80: E501 line too long (109 > 79 characters)
Line 50:80: E501 line too long (110 > 79 characters)
Line 51:80: E501 line too long (109 > 79 characters)
Line 52:80: E501 line too long (109 > 79 characters)
Line 55:80: E501 line too long (111 > 79 characters)
Line 58:80: E501 line too long (111 > 79 characters)
Line 61:80: E501 line too long (111 > 79 characters)
Line 64:80: E501 line too long (111 > 79 characters)
Line 66:80: E501 line too long (110 > 79 characters)
Line 67:80: E501 line too long (109 > 79 characters)
Line 68:80: E501 line too long (110 > 79 characters)
Line 70:80: E501 line too long (109 > 79 characters)
Line 71:80: E501 line too long (110 > 79 characters)
Line 72:80: E501 line too long (109 > 79 characters)
Line 73:80: E501 line too long (109 > 79 characters)
Line 74:80: E501 line too long (110 > 79 characters)
Line 75:80: E501 line too long (109 > 79 characters)
Line 76:80: E501 line too long (109 > 79 characters)
Line 79:80: E501 line too long (111 > 79 characters)
Line 82:80: E501 line too long (111 > 79 characters)
Line 85:80: E501 line too long (111 > 79 characters)

@mkcor mkcor marked this pull request as ready for review March 18, 2021 16:38
@mkcor
Copy link
Member Author

mkcor commented Mar 18, 2021

The current test fail is not specific to this PR.

@hmaarrfk @BierretA I have manually cherry-picked the new (non-duplicate) data files from #5234.

@mkcor
Copy link
Member Author

mkcor commented Mar 20, 2021

@hmaarrfk please review/approve ;)

Copy link
Member

@hmaarrfk hmaarrfk left a comment

Choose a reason for hiding this comment

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

It seems that adding the files to the registry itself causes test to fail.

I noticed that the azure pipelines all build off the build wheels.

They will fail without a pull request like this.

@hmaarrfk
Copy link
Member

I'm kinda ok with this, but I think that merging #5283 would have caused fewer failures.

I can't really inspect the azure failures, but in either case, it is clear that the build system has stopped that PR from going through.

I would be ok just merging the main PR as is.

@mkcor mkcor mentioned this pull request Mar 22, 2021
@mkcor
Copy link
Member Author

mkcor commented Mar 22, 2021

It seems that adding the files to the registry itself causes test to fail.

Yes, because the files don't live in main yet! So, if we merge this PR first, then we can go on with the data registry update (via a PR which will pass CI).

@mkcor
Copy link
Member Author

mkcor commented Mar 22, 2021

I'm kinda ok with this, but I think that merging #5283 would have caused fewer failures.

@hmaarrfk "would cause" 😉 Let's fix the CI script first, of course!

@mkcor
Copy link
Member Author

mkcor commented Mar 30, 2021

Let's move forward with this, now that #5283 is merged! /cc @grlee77 @jni

@grlee77 grlee77 merged commit 1874ced into scikit-image:main Mar 30, 2021
@mkcor mkcor deleted the data-files-illuminants branch April 6, 2021 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants