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

Extensions of FileIO interface #373

Merged
merged 14 commits into from
Sep 29, 2015
Merged

Extensions of FileIO interface #373

merged 14 commits into from
Sep 29, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 27, 2015

I excised ec2dec5, reverted the commit (#370) that caused the test failure, created some deprecations, and moved Imagine into its own package.

Only AndorSIF to go, right? Do you want me to tackle that?

SimonDanisch and others added 8 commits September 27, 2015 17:54
This was only for ImageMagick, see JuliaIO/ImageMagick.jl#7
@timholy the Imagine port is incomplete because:
- don't know file ending
- don't know magic byte
- would need to implement submodule loading in Fileio (own packige for
Imagine?)
you should probably just do the changes you feel to be appropriate.
@SimonDanisch
Copy link
Member

Awesome!!!
AndorSIF is mostly ready and it might be better if you do the rest. I don't know the format and couldn't find any files to test this. I think it's fine to leave it in Images, as the other IO packages rely on Images anyways. Up to you ;)

@timholy
Copy link
Member Author

timholy commented Sep 27, 2015

OK. @rsrock, I'm going to take the liberty of moving the SIF reader over to JuliaIO, and make sure you have push privileges.

@timholy
Copy link
Member Author

timholy commented Sep 27, 2015

https://github.com/JuliaIO/AndorSIF.jl

Once this passes its tests, I'm merging 😄.

@SimonDanisch
Copy link
Member

Ha! That would be quite something :)
Just remembered though, that the reading Vector{Uint8} hasn't made it into ImageMagick.jl yet.
Are we forgetting other stuff?

@SimonDanisch SimonDanisch mentioned this pull request Sep 27, 2015
5 tasks
@SimonDanisch
Copy link
Member

Win64 0.3 is off by one:

Expression: raw(res) --> raw(map(ScaleAutoMinMax(Ufixed16),raw(imgr))) 
Expected: 
Uint16[65535 43131
       49330 23688
       35780 41243]
Uint16[40516 36945
       43380 18967
       28964 0]
Occurred: 
Uint16[65535 43131
       49330 23688
       35780 41243]
Uint16[40516 36945
       43379 18967
       28964 0] 

This must be a rounding error due floating point precision problems?
Should we relax the test? Would be annoying if this lets the tests fail at random

@timholy
Copy link
Member Author

timholy commented Sep 27, 2015

I want to try to debug it first, so for now let's have it generate enough output to reproduce the problem.

@timholy
Copy link
Member Author

timholy commented Sep 28, 2015

The writemime test failure is expected until JuliaIO/FileIO.jl#37 gets merged.

@rsrock
Copy link
Collaborator

rsrock commented Sep 28, 2015

No problem, I'll catch up with all these changes when I have the chance. I assume osxnative is moving somewhere else too? Let me know where that will go, I'd like to add image writing too.

I doubt that Andor SIF is used much. At one point I tried to grab a small test image, but it was with the wrong version of the software and the format was different. Of course.

Ronald S. Rock, Jr., Associate Professor
Dept. of Biochemistry and Molecular Biology, The University of Chicago
GCIS W240, 929 E. 57th St.
Chicago, IL 60637 USA
+1 773.702.0716 (w), +1 773.702.0439 (f)
rrock@uchicago.edu

On Sep 27, 2015, at 2:47 PM, Tim Holy notifications@github.com wrote:

OK. @rsrock, I'm going to take the liberty of moving the SIF reader over to JuliaIO, and make sure you have push privileges.


Reply to this email directly or view it on GitHub.

@timholy timholy mentioned this pull request Sep 28, 2015
@SimonDanisch
Copy link
Member

Little checklist:

  • SIF
  • Imagine
  • pass PBM tests
  • writemime
  • rounding (?) error
  • OSXNative
  • OSXNative rename
  • NRRD
  • ImageMagick
  • merge Choose a default for ambiguous in FileIO

Probably incomplete ;)

@timholy
Copy link
Member Author

timholy commented Sep 28, 2015

OK, I think we're basically good to go here. This couples to the following PRs:

I think lots of travis tests will fail (surprisingly, this one succeeded) until packages are registered and tagged. OK to go ahead?

@timholy
Copy link
Member Author

timholy commented Sep 28, 2015

(I'm not sure what the "rounding error" means above.)

@SimonDanisch
Copy link
Member

This one:

Expression: raw(res) --> raw(map(ScaleAutoMinMax(Ufixed16),raw(imgr))) 
Expected: 
Uint16[65535 43131
       49330 23688
       35780 41243]
Uint16[40516 36945
       43380 18967
       28964 0]
Occurred: 
Uint16[65535 43131
       49330 23688
       35780 41243]
Uint16[40516 36945
       43379 18967
       28964 0] 

@timholy
Copy link
Member Author

timholy commented Sep 28, 2015

Ah, thanks for reminding me. Yes, let's wait for it to happen again and see what the debugging output tells us.

timholy added a commit that referenced this pull request Sep 29, 2015
Extensions of FileIO interface
@timholy timholy merged commit 416af79 into master Sep 29, 2015
@timholy timholy deleted the sd_teh/fileio2 branch September 29, 2015 10:02
@timholy
Copy link
Member Author

timholy commented Sep 29, 2015

Aside from a weird problem with QuartzImageIO, things are looking good. I'm interested in tagging a new (minor) release very soon. A key question: should Images (temporarily?) add

ImageMagick
@osx QuartzImageIO

to its REQUIRE file?

Also, if folks can test I'd be grateful.

@SimonDanisch
Copy link
Member

Wow, is it Christmas already!? :)
On 29 Sep 2015 12:02, "Tim Holy" notifications@github.com wrote:

Merged #373 #373.


Reply to this email directly or view it on GitHub
#373 (comment).

@vchuravy
Copy link

💯 Whoo

@lucasb-eyer
Copy link
Collaborator

A Pkg.update(), followed by Pkg.checkout("Images") also installs FileIO. Subsequent Pkg.test("Images") succeeds. imread("some.png") nicely warns about deprecated imread and recommends load.

But with the new system, I'm back to having ImageMagick problems:

julia> load("/home/beyer/zealgo.png")
ERROR: ArgumentError: ImageMagick not found in path
 in require at ./loading.jl:233
 in check_loader at /home/beyer/.julia/v0.5/FileIO/src/loadsave.jl:11

A Pkg.free("Images") "fixes" this problem, e.g. imread works on my system in the latest release. I'll investigate this towards the end of the week.

Edit to clarify: I'm not sure it's this specific patchset that introduces the regression or any other since the last release, so the problem might well be unrelated!

@timholy
Copy link
Member Author

timholy commented Sep 29, 2015

Thanks for checking. Right now you have to say Pkg.add("ImageMagick"). See #373 (comment).

@lucasb-eyer
Copy link
Collaborator

Ah, right, I read that comment but comitted it to memory with a bitflip :) Adding the ImageMagick package, it all works fine on my Arch Linux for a few images I tried.

@timholy
Copy link
Member Author

timholy commented Sep 29, 2015

Awesome! Thanks for the report.

I think we can take this as evidence that we should add it to the REQUIRE 😄.

@lucasb-eyer
Copy link
Collaborator

That, or to the README.md if you want to force people to choose their loader.

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.

None yet

5 participants