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

Rename package and redesign types #5

Merged
merged 3 commits into from
May 17, 2022
Merged

Rename package and redesign types #5

merged 3 commits into from
May 17, 2022

Conversation

timholy
Copy link
Member

@timholy timholy commented May 15, 2022

This creates an abstract AbstractColorChannels type and two types,
ColorChannels and ColorMixture. ColorChannels is a "bare"
multichannel color that lacks conversion to other color types;
it is intended to be used in conjunction with MappedArrays if one
wishes to visualize these as RGB images. ColorMixture is the
weighted-RGB color type.

Closes #2

@codecov
Copy link

codecov bot commented May 15, 2022

Codecov Report

Merging #5 (a4d7db9) into main (21e7a6b) will decrease coverage by 7.07%.
The diff coverage is 61.29%.

@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
- Coverage   67.07%   60.00%   -7.08%     
==========================================
  Files           6        6              
  Lines          82       95      +13     
==========================================
+ Hits           55       57       +2     
- Misses         27       38      +11     
Impacted Files Coverage Δ
src/types.jl 65.21% <60.00%> (-19.64%) ⬇️
src/MultiChannelColors.jl 100.00% <100.00%> (ø)

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 21e7a6b...a4d7db9. Read the comment docs.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

The conceptual changes look good to me. Although I'm not sure if AbstractColorChannels and ColorChannels (plural form) are good names for types -- we usually use plural forms for modules. How about AbstractMultiChannelColor and MultiChannelColor?

We might need to support alias for specific channels -- I'm thinking of something like this:

const MColor2Channels = (RGB(0, 0.5, 0.5), RGB(1, 0, 0))
const MColor2{T} = ColorMixture{T,2,MColor2Channels}

MColor2{Float32}(0.8, 0.2) # equivalent to `ColorMixture(MColor2Channels, (0.8, 0.2))`

For ColorMixture, to avoid confusions. we need to clearly document that it is not RGB colorspace:

  • length(T) is not 3
  • mapc applies to the original channels and not to the RGB channels
  • etc

This creates an abstract `AbstractColorChannels` type and two types,
`ColorChannels` and `ColorMixture`. `ColorChannels` is a "bare"
multichannel color that lacks conversion to other color types;
it is intended to be used in conjunction with MappedArrays if one
wishes to visualize these as RGB images. `ColorMixture` is the
weighted-RGB color type.

The package name has been changed to reflect the wider applicability.
This also adds Documenter documentation and slims the README.
@timholy
Copy link
Member Author

timholy commented May 17, 2022

Addressed the points above.

Now with greatly expanded documentation. The docs talk about arithmetic operations, but those will be implemented in the next PR.

docs/make.jl Outdated Show resolved Hide resolved
docs/src/faq.md Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
The color types in this package support two fundamental categories of operations:

- arithmetic operations such as `+` and `-` and multiplying or dividing by a scalar. You can also scale each color channel independently with `⊙` (obtained with `\odotTAB`) or its synonym `hadamard`, e.g., `g ⊙ c` where `c` is a color object defined in this package and `g` is a tuple of real numbers (the "gains").
- extracting the independent channel intensities as a tuple with `Tuple(c)`.
Copy link
Member

Choose a reason for hiding this comment

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

TIL 😆

docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved

Use the package interactively or in code with

```jldoctest demo
Copy link
Member

Choose a reason for hiding this comment

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

Just a confirmation: is the jldoctest deliberately used here rather than the @repl block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though there are many ways to solve the same problem. Basically I want doctests to make sure the code keeps working, and I don't want to have to use setup=:(using MultiChannelColors) on each doctest. So I created a named doctest, demo, and am loading the package in the first instance.

julia> typeof(ctemplate)
ColorMixture{Float32, 2, (RGB{N0f8}(0.0,1.0,0.0), RGB{N0f8}(1.0,0.0,0.0))}

julia> c = ctemplate(0.2, 0.4)
Copy link
Member

Choose a reason for hiding this comment

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

neat!

```julia
A = rand(0x0000:0x0fff, 100, 100, 11);
img = StructArray{MultiChannelColor{N4f12,11}}(A; dims=3)
```
Copy link
Member

Choose a reason for hiding this comment

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

If we provide a better tool in ImageCore in the spirit of JuliaImages/ImageCore.jl#179, then this story might be changed.
Might be worth leaving a note about future improvement.


I was thinking of something like:

module ImageLayouts
const CHW = DenseImageLayout{:CHW}()
const CWH = DenseImageLayout{:CWH}()
const HWC = DenseImageLayout{:HWC}()
end

so that users can use a unified interface:

using ImageCore # which exports the ImageLayouts module

colorview(ImageLayouts.CHW, img)

we might end up with a new function, though -- pretty much like the strel concept I introduced in ImageMorphology.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I really like the idea of these custom layouts!

If you'll be hard-coding the name of the fluorophore, consider using a slightly different syntax:

```jldoctest demo
julia> channels = (fluorophore_rgb"EGFP", fluorophore_rgb"tdTomato")
Copy link
Member

@johnnychen94 johnnychen94 May 17, 2022

Choose a reason for hiding this comment

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

Will fluorophore"EGFP" be easier to use? I'm not sure. -- just that I have never seen underscores used in string macros.

Is there non-RGB fluorophore?

Copy link
Member Author

Choose a reason for hiding this comment

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

No non-RGB fluorophore currently. I guess I would think that fluorophore"EGFP" might return an entire object that is a complete characterization of the fluorophore, e.g., full spectra, cross section, quantum yield, and so on (this list only scratches the surface). In contrast, fluorophore_rgb makes it clear that what you're getting is an RGB value that represents the fluorophore.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

I believe this is my last round review. It looks great to me so feel free to merge when you think ready.

@timholy timholy merged commit 660f5de into main May 17, 2022
@timholy timholy deleted the teh/redesign branch May 17, 2022 21:39
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.

generalize the ColorMixture concept
2 participants