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 support for extra channels #159

Merged
merged 6 commits into from
Oct 21, 2024
Merged

Conversation

chrstphrbrns
Copy link
Contributor

Fixes #100

Exports three new functions

  • color -- extract the color component of an image, eg
julia> imshow(color(im));
  • nchannels -- get the total number of channels from an image (color channels + extra channels)
  • channel -- get the values from a specific channel (with the first N channels being color channels), eg
julia> imshow(channel(im, 5));

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 87.87879% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.36%. Comparing base (25cf338) to head (31eaaa3).

Files with missing lines Patch % Lines
src/types/common.jl 85.71% 5 Missing ⚠️
src/types/widepixel.jl 28.57% 5 Missing ⚠️
src/load.jl 87.50% 1 Missing ⚠️
src/types/lazy.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   92.68%   92.36%   -0.33%     
==========================================
  Files          13       14       +1     
  Lines        1121     1152      +31     
==========================================
+ Hits         1039     1064      +25     
- Misses         82       88       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@tlnagy tlnagy left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Just a couple smaller points.

test/layouts.jl Show resolved Hide resolved
test/mmap_lazyio.jl Show resolved Hide resolved
src/types/widepixel.jl Show resolved Hide resolved
src/types/widepixel.jl Outdated Show resolved Hide resolved
@chrstphrbrns chrstphrbrns deleted the chris/wide-pixel branch April 11, 2024 16:07
@chrstphrbrns chrstphrbrns restored the chris/wide-pixel branch April 11, 2024 16:07
@chrstphrbrns chrstphrbrns reopened this Apr 11, 2024
Copy link
Owner

@tlnagy tlnagy left a comment

Choose a reason for hiding this comment

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

This is basically ready to merge, I think only the test for #56 is missing.

test/layouts.jl Show resolved Hide resolved
@kimikage
Copy link
Contributor

This is just a comment and does not block the merge, but color is one of the functions that ColorTypes exports.
TiffImages users are likely to be using ColorTypes, Colors, or Images.
At the very least, it would be good to have a note in the document.

@chrstphrbrns
Copy link
Contributor Author

This is just a comment and does not block the merge, but color is one of the functions that ColorTypes exports. TiffImages users are likely to be using ColorTypes, Colors, or Images. At the very least, it would be good to have a note in the document.

What kind of a note? Julia already warns when two packages export the same symbol

WARNING: both ColorTypes and TiffImages export "color"; uses of it in module Main must be qualified

@kimikage
Copy link
Contributor

It is not what it is, but where it is that matters.

in the document.

@tlnagy
Copy link
Owner

tlnagy commented May 20, 2024

Sorry for the delay here, I was on vacation. I'm happy with the state of this PR (thanks @chrstphrbrns for your hard work here!). I'm going to release #165 as v0.10.1 and then I'll merge this PR into master to be released as v0.11.0.

@juliohm
Copy link

juliohm commented Aug 2, 2024

@tlnagy any update regarding this amazing PR by @chrstphrbrns ?

@juliohm
Copy link

juliohm commented Oct 10, 2024

It would be really useful to have this feature in place to solve #100

@tlnagy what is your plan regarding this PR?

@tlnagy
Copy link
Owner

tlnagy commented Oct 10, 2024

Sorry, I've been swamped at my new job and haven't had much time for my Julia packages. I just released v0.10.1 and I'll release #165 as v0.10.2, rebase this branch, and if it all passes, this'll come out as v0.11.0

@tlnagy
Copy link
Owner

tlnagy commented Oct 10, 2024

The segmentation fault with Julia nightly on MacOS + Arm64 is concerning. @chrstphrbrns any idea why SIMD might be failing there?

@juliohm
Copy link

juliohm commented Oct 17, 2024

@tlnagy is the failure on Julia nightly blocking? We are really looking forward to this.

@eliascarv
Copy link

eliascarv commented Oct 21, 2024

@tlnagy, reading the error stack trace, we can see that the error is coming from the SIMD.jl package:

Captura de tela de 2024-10-21 08-46-25

Reading the TiffImages.jl code, I noticed that the deplane! and recode functions use SIMD.jl, these are used by the read! function, which is used by the load function.

The SIMD.jl package uses internal Julia APIs, which may change between versions. This is why we may notice that SIMD.jl tests break in Julia nightly:

image

So I think we can conclude that the errors in the tests are related to the SIMD.jl package, not the PR. Do you agree?

Maybe tests should only be run on stable Julia versions?

@eliascarv
Copy link

And about the test that is breaking on Ubuntu with Julia 1.11, I ran the PR tests locally on my machine with Fedora and Julia 1.11.1 and the tests passed normally:

image

Version info:

Julia Version 1.11.1
Commit 8f5b7ca12ad (2024-10-16 10:53 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 12 × Intel(R) Core(TM) i5-10400 CPU @ 2.90GHz
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, skylake)
Threads: 1 default, 0 interactive, 1 GC (on 12 virtual cores)

@tlnagy
Copy link
Owner

tlnagy commented Oct 21, 2024

Maybe tests should only be run on stable Julia versions?

This a good point. I was mostly concerned about it failing on M series chips, but yeah it should only be blocking on stable Julia.

@tlnagy
Copy link
Owner

tlnagy commented Oct 21, 2024

And about the test that is breaking on Ubuntu with Julia 1.11, I ran the PR tests locally on my machine with Fedora and Julia 1.11.1 and the tests passed normally:

The failure is on 32 bit systems so it's likely related to #169 and #170 so it shouldn't hold anything up.

@tlnagy tlnagy merged commit 4768572 into tlnagy:master Oct 21, 2024
13 of 14 checks passed
@tlnagy
Copy link
Owner

tlnagy commented Oct 21, 2024

Huge thanks for @chrstphrbrns for all the work getting this done and @juliohm for consistently advocating for it. Excited to have this in TiffImages v0.11.0

@tlnagy tlnagy mentioned this pull request Oct 21, 2024
@juliohm
Copy link

juliohm commented Oct 23, 2024

Thank you @tlnagy for making it happen! Do you have a date for the new v0.11 release? We are looking forward to it.

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.

Add support for GeoTIFFs
6 participants