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

partially added 16-bit tiff support #7731

Merged
merged 4 commits into from May 31, 2019

Conversation

@BioinfoTongLI
Copy link

commented May 14, 2019

What changes were proposed in this pull request?

To be able to load 16-bit multipage tiff file into INDArray

How was this patch tested?

unit test with data in testresource

Quick checklist

The following checklist helps ensure your PR is complete:

  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.
  • Ran mvn formatter:format (see formatter instructions for targeting your specific files).
trim multipage option to MINIBATCH and FIRST only, since more complex…
… data structure needs more metadata given by user. That is not supported for instance.
@BioinfoTongLI

This comment has been minimized.

Copy link
Author

commented May 17, 2019

Sorry for being laggy for this small changes.
I'm preparing my thesis defense. Kind of busy.
BTW, I saw the tests are not passing. I believe it's due to the PR in testresource.
As for why that is not passing. I have no idea... Any clue?

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@BioinfoTongLI I'm seeing all tests pass in datavec-data-image on your branch when tested locally (using latest master for dl4j-test-resources and mono repo as base)... including your new tests.
That's good enough for me... don't worry about any other failures not related to your changes.
So this is good to go then?

@BioinfoTongLI

This comment has been minimized.

Copy link
Author

commented May 17, 2019

I saw that too. Wired.... But Yeah, ok for me to go.

@AlexDBlack
Copy link
Contributor

left a comment

Only minor criticism is the old commented out code... that should be cleaned up.
But we can fix that up later, not a big deal.

@AlexDBlack AlexDBlack requested a review from saudet May 17, 2019

@BioinfoTongLI

This comment has been minimized.

Copy link
Author

commented May 20, 2019

I don't get it.
If I take the channels into account. It will be
data = Nd4j.create(pixa.n()/n_ch, n_ch, 1, pixa.pix(0).h(), pixa.pix(0).w());, right?
And for images with more than 1 channel. How can I know its[c1, c1, c1, c1, c2, c2, c2, c2]
or [c1, c2, c1, c2, c1, c2, c1, c2]? Tell me if I miss something.

@saudet

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I'm pretty sure the number of channels and the number of pages are not related to each other... Anyway, if nobody complains about this, let's just merge it as is I suppose.

@saudet
saudet approved these changes May 20, 2019

@saudet saudet changed the base branch from master to dev_20190513 May 31, 2019

@saudet saudet merged commit 16f1e64 into eclipse:dev_20190513 May 31, 2019

0 of 2 checks passed

Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
continuous-integration/jenkins/pr-head This commit is being built
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.