search

JuliaGraphics / ColorTypes.jl

Basic color definitions and traits
Other
77 stars 35 forks source link

Implement `<` in addition to `isless` #213

Closed timholy closed 3 years ago

timholy commented 3 years ago

Note: this replaces #186 for <. I'll add the rest in separate PRs.

160 "moved" isless from ColorVectorSpace to ColorTypes, but didn't likewise move <. Because < falls back to isless,

this meant that comparisons like Gray(0.3) < Gray(NaN) returned a different answer than 0.3 < NaN.

This is breaking, because moving methods from one package to another is breaking unless all compatible versions of the "upstream" package (ColorVectorSpace) were designed with the possibility that these methods would be present in this package. That's not the case, so we need to introduce an incompatibility. Worth nothing that if we put ColorTypes, Colors, and ColorVectorSpace into a single multi-package repository, we might be able to such a transfer in a non-breaking way.

codecov[bot] commented 3 years ago

Codecov Report

Merging #213 into master will increase coverage by 0.18%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   94.10%   94.29%   +0.18%     
==========================================
  Files           8        8              
  Lines         696      701       +5     
==========================================
+ Hits          655      661       +6     
+ Misses         41       40       -1
Impacted Files Coverage Δ
src/ColorTypes.jl 100.00% <ø> (ø)
src/traits.jl 98.42% <100.00%> (+0.58%) :arrow_up:

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 6d2759c...ec2e51c. Read the comment docs.

kimikage commented 3 years ago

Generally, this is a breaking change. However, as I wrote in https://github.com/JuliaGraphics/ColorTypes.jl/pull/186#issuecomment-672503156, I think this can be included in v0.10. Of course, I do not intend to actively take risks.

timholy commented 3 years ago

I was responding to that comment. Here's the issue:

julia> using ColorVectorSpace
[ Info: Precompiling ColorVectorSpace [c3611d14-8923-5661-9e6a-0046d554d3a4]
WARNING: Method definition <(ColorTypes.Color{T, 1} where T, ColorTypes.Color{T, 1} where T) in module ColorTypes at /home/tim/.julia/dev/ColorTypes/src/traits.jl:521 overwritten in module ColorVectorSpace at /home/tim/.julia/packages/ColorVectorSpace/B9KU5/src/ColorVectorSpace.jl:259.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition <(ColorTypes.Color{T, 1} where T, Real) in module ColorTypes at /home/tim/.julia/dev/ColorTypes/src/traits.jl:522 overwritten in module ColorVectorSpace at /home/tim/.julia/packages/ColorVectorSpace/B9KU5/src/ColorVectorSpace.jl:260.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition <(Real, ColorTypes.Color{T, 1} where T) in module ColorTypes at /home/tim/.julia/dev/ColorTypes/src/traits.jl:523 overwritten in module ColorVectorSpace at /home/tim/.julia/packages/ColorVectorSpace/B9KU5/src/ColorVectorSpace.jl:261.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition isless(ColorTypes.Color{T, 1} where T, Real) in module ColorTypes at /home/tim/.julia/dev/ColorTypes/src/traits.jl:518 overwritten in module ColorVectorSpace at /home/tim/.julia/packages/ColorVectorSpace/B9KU5/src/ColorVectorSpace.jl:265.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition isless(Real, ColorTypes.Color{T, 1} where T) in module ColorTypes at /home/tim/.julia/dev/ColorTypes/src/traits.jl:519 overwritten in module ColorVectorSpace at /home/tim/.julia/packages/ColorVectorSpace/B9KU5/src/ColorVectorSpace.jl:266.
  ** incremental compilation may be fatally broken for this module **

We can change ColorVectorSpace to checked whether the methods are already defined, but that's a new release of ColorVectorSpace. What if the user has an older version of ColorVectorSpace and a newer version of ColorTypes?

kimikage commented 3 years ago

What if the user has an older version of ColorVectorSpace and a newer version of ColorTypes?

pkg> update

This or similar case can happen at any time with bug fixes.

timholy commented 3 years ago

But the idea behind the package manager is that it shouldn't, if folks use bounds properly.

kimikage commented 3 years ago

We can't (and don't want to) revert or make maintenance branches too casually, so I just did a final check. My focus is on next minor updates of Colors and FixedPointNumbers. Essentially, each package can be updated asynchronously, but that is not the case in practice. :confused:

If the next minor release of ColorVectorSpace should be achieved early, Colors should add the support ColorTypes v0.11 at the patch level (e.g. Colors v0.12.~4~x). Therefore, in that case, ColorTypes v0.11 would be better off focusing only on the breaking changes related to ColorVectorSpace.

timholy commented 3 years ago

I agree that we'd really rather not be maintaining more than one branch. But unless we've introduced a regression, can't we just say to users of older branches "figure out how to upgrade or make a maintenance PR yourself"? That is, we don't have to fix all conceivable bugs on 0.10 before we switch our focus to 0.11.

And yes, we can do patch-level bumps in higher-level packages.

The main reason I'd like to tackle this is that it makes sense to get the basic mathematical operations in the color packages squared away before beginning some shakeups in JuliaImages. I've been busy with invalidations but that's pretty much wrapped up now (i.e., blog post will be published probably next week and at that point the whole community can contribute, not that all work on eliminating invalidations is done). So I'd like to put some more time into JuliaImages, just wondering whether I can do what I need to do here first or whether I should direct my attention elsewhere. If it's really inconvenient to fork 0.11 now, I'll focus on ImageCore and come back when you're ready.

kimikage commented 3 years ago

My concern is with the endpoint of v0.11, not the start. I have no inconvenience in merging breaking changes into the current master branch.

For example, PR #206 can affect the tests and documentation (cf. https://github.com/JuliaTesting/ReferenceTests.jl/pull/66). If we merge it in v0.11 series, I think ColorTypes v0.11 should be supported in Colors v0.13.0 instead of v0.12.~4~x. However, and then, the CompatHelper festival will start again.:smile:

Edit: WRT breaking changes, Colors.jl stays clean. In other words, I intentionally keep Colors in the clean state to allow for both v0.12.~4~x and v0.13.0 to be selectable.

However, does ColorVectorSpace actually depend on Colors in the first place?

timholy commented 3 years ago

I saw your edit:

However, does ColorVectorSpace actually depend on Colors in the first place?

Directly, almost not at all. But JuliaImages loads ColorTypes, Colors, and ColorVectorSpace. In a sense, Colors is the least important of them, but colorspace conversions are used in a few places.

kimikage commented 3 years ago

Most of the implementations of colorspace conversions can be found in Colors.jl, but the "public" API is defined in ColorTypes.jl. My concern is that the upper bound on Colors.jl may be too strict.

kimikage commented 3 years ago

This was already scheduled two months ago, so I will merge this.