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.

Sign up for GitHub

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

Jump to bottom

[RFC] (Re-)define one for Colorant #243

Merged
merged 1 commit into from
May 14, 2021
Merged

[RFC] (Re-)define one for Colorant #243

merged 1 commit into from
May 14, 2021

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Apr 3, 2021 •
edited
Loading

This defines one as a real (i.e. dimensionless) number １.

I am not convinced that this is the best way, but I think this is a reasonable option.

Since this does not support ones(ARGB, ...) and so on, this may requires PR #177 as a part of v0.11.0

Closes #235, Closes #137

@kimikage kimikage marked this pull request as draft April 3, 2021 13:39
@codecov
Copy link

codecov bot commented Apr 3, 2021 •
edited
Loading

Codecov Report

Merging #243 (19629f0) into master (213e0de) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   82.62%   82.73%   +0.10%     
==========================================
  Files           8        8              
  Lines         754      753       -1     
==========================================
  Hits          623      623              
+ Misses        131      130       -1
Impacted Files Coverage Δ
src/ColorTypes.jl 100.00% <ø> (ø)
src/error_hints.jl 100.00% <100.00%> (+10.00%) ⬆️
src/traits.jl 98.88% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 213e0de...19629f0. Read the comment docs.

@kimikage kimikage marked this pull request as ready for review April 3, 2021 15:41
@kimikage kimikage requested review from johnnychen94 and timholy April 3, 2021 15:45
@kimikage kimikage mentioned this pull request Apr 3, 2021
Merged
10 tasks
johnnychen94
johnnychen94 approved these changes Apr 4, 2021
View reviewed changes
Copy link
Member

@johnnychen94 johnnychen94 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

No objections at all!

Remind me, what's the plan on ColorVectorSpaces? Will there be any zero/one/oneunit methods there? I guess no?

@kimikage
Copy link
Collaborator Author

kimikage commented Apr 4, 2021

I'm not sure I fully understand what @timholy wants to do with ColorVectorSpace v0.9. For now, ColorVectorSpace is allowing breaking changes to remain at v0.9, rather than "skipping" to ColorVectorSpace v0.10. (Incidentally, I'd rather there be no gaps than to strictly adhere to the SemVer principles.)

The options can be roughly divided into two approaches.

One is that ColorTypes v0.11 will not include this PR, and only define zero and oneunit (and <). This is a "formally" safe approach, but "in practice" it may cause confusion about the support for ColorVectorSpace v0.9 in the downstream packages.

The other is to include this PR and PR #177 in ColorTypes v0.11 and break the definition of one in ColorVectorSpace v0.9.x, and drop the support for ColorTypes v0.10. This seems to be a hard landing, but the package manager should avoid the crash.:sweat_smile:

@kimikage
Copy link
Collaborator Author

kimikage commented Apr 8, 2021

In the meantime, I will make some trivial PRs for ColorVectorSpace.

@kimikage
Copy link
Collaborator Author

kimikage commented Apr 21, 2021 •
edited
Loading

ImageCore v0.9.0, which depends on ColorVectorSpace v0.9, has been released. Currently it should not be available yet, but some packages will start supporting ColorVectorSpace v0.9. Therefore, releasing ColorVectorSpace v0.10 may be the right way to go.

@kimikage
Copy link
Collaborator Author

kimikage commented Apr 22, 2021 •
edited
Loading

I've updated the plan: #231. I'll merge PR #243 #246 and another migration PR (Edit: #248), and release v0.11.0 by the beginning of next week. v0.12.0 is a bit heavier, but I hope to release it by the end of May. I think FixedPointNumbers v0.9 will be the next target.
Of course, this is a personal observation and not a definitive one, for better or worse.

@kimikage kimikage added this to the 0.12 milestone Apr 22, 2021
@kimikage
Copy link
Collaborator Author

Ah, the error hints for color arithmetic may be v0.11 material. I'm going to split the PR further.

@johnnychen94
Copy link
Member

johnnychen94 commented Apr 22, 2021 •
edited
Loading

Since error hints don't change the behavior so it's okay to ship it in v0.12 just to save some effort; I'll just treat this laziness as "whoever wants the new fancy feature, use the new version."

@kimikage kimikage mentioned this pull request Apr 24, 2021
Merged
@kimikage
Copy link
Collaborator Author

I have submitted a separated PR #249. I am going to remove duplicate changes when I apply rebase to this PR. That is, I will leave this PR as is for a while.

This was referenced Apr 24, 2021
Closed
Open
@kimikage kimikage mentioned this pull request May 6, 2021
Closed
19 tasks
@kimikage
(Re-)define one for Colorant
19629f0 
This defines `one` as a real (i.e. dimensionless) number `１`.
@kimikage kimikage merged commit 306ed7d into JuliaGraphics:master May 14, 2021
@kimikage kimikage deleted the define_one branch May 14, 2021 16:28
@kimikage kimikage mentioned this pull request May 14, 2021
Closed
@kimikage kimikage mentioned this pull request Aug 13, 2021
Closed
johnnychen94 added a commit that referenced this pull request May 15, 2022
@johnnychen94
Revert "(Re-)define one for Colorant (#243)"
444307d 
This reverts commit 306ed7d.
@kimikage kimikage mentioned this pull request Apr 23, 2024
Open
kimikage added a commit to kimikage/ColorTypes.jl that referenced this pull request May 6, 2024
@kimikage
Remove broken test_broken for ones
aa6b50b 
PR JuliaGraphics#243 and JuliaGraphics#177 had already fixed the `ones` throwing an exception.
This updates the test.
@kimikage kimikage mentioned this pull request May 6, 2024
Merged
kimikage added a commit that referenced this pull request May 6, 2024
@kimikage
Remove broken test_broken for ones (#308)
e195457 
PR #243 and #177 had already fixed the `ones` throwing an exception.
This updates the test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnnychen94 johnnychen94 johnnychen94 approved these changes

@timholy timholy Awaiting requested review from timholy

Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.

[RFC] Decision on the definition of one support ones(::Type{<:AbstractGray}, sz)
2 participants
@kimikage @johnnychen94