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

drop julia 0.5, view keyword arg, and 16-bit tiffs #103

Merged
merged 3 commits into from
Aug 24, 2017

Conversation

bjarthur
Copy link
Member

closes #102

have load return a view instead of using permutedims. note that there are further optimizations to be had here, by writing in-place versions of flipdim.

and separately fix saving of 16-bit tiffs. interestingly, even on master the "binary png" and "alpha" tests are failing. have not taken the time to git bisect when this breakage occurred.

@timholy
Copy link
Member

timholy commented Aug 18, 2017

I suppose we should do this. The one reason I hesitate here is that I think ImageMagick doesn't do anything special to load truly large files. For example, in NRRD and ImagineFormat we use mmap to load files that can be much, much larger than the available memory. If using ImageMagick, it seems that IM has to be able to load the whole file into a buffer in RAM. As a consequence, the best case scenario for this PR is that it lets you load a 2x larger image. Is that narrow "window" worth it? I worry that even if we make this operation work, it's not unlikely that the very next operation the user executes on the image might fail. In contrast, shopping for more RAM is an effective solution for many problems.

@timholy
Copy link
Member

timholy commented Aug 18, 2017

Again, I'm not strongly against this (indeed I might on balance be slightly in favor), but I do have some reluctance to slow down operations for all the 2d computer vision people who work with small images.

@bjarthur
Copy link
Member Author

is it too much to ask those with small files to collect(load(...)) ?

alternatively, what about using a keyword argument to specify a view, like load(filename, view=true), with the default being false.

@timholy
Copy link
Member

timholy commented Aug 18, 2017

I like the keyword argument idea, I think that makes sense.

@bjarthur
Copy link
Member Author

keyword arg added.

but oi, PermutedDimsArray is not in Compat.jl it seems, so this PR will not work on julia 0.5. i might just be too lazy to merge until after we drop support.

@bjarthur
Copy link
Member Author

hah, just noticed Images.jl has dropped support for 0.5. should we do the same for ImageMagick?

also, has it been discussed at all to add a FlippedDimArray to julia Base to match flipdim, much like PermutedDimArray matches permutedims? that would nicely round out this PR.

@timholy
Copy link
Member

timholy commented Aug 22, 2017

Sure, dropping support for 0.5 seems very reasonable. I bumped all the JuliaImages packages so that I could run femtocleaner to eliminate depwarns on 0.7 (not that I'm using it in practice yet).

With regards to FlippedDimArray, you can get that with things along the lines of view(img, reverse.(indices(img))...).

@bjarthur bjarthur force-pushed the bja/view16 branch 2 times, most recently from 7957ffa to 8138dd1 Compare August 23, 2017 11:37
@bjarthur
Copy link
Member Author

doh! thanks.

flipdim has been replaced with a view (which can be collected with the same view keyword argument).

and julia 0.5 support has been dropped. should remember to bump the minor version number for the next tag

@bjarthur bjarthur changed the title views and 16-bit tiffs drop julia 0.5, views, and 16-bit tiffs Aug 23, 2017
@bjarthur bjarthur changed the title drop julia 0.5, views, and 16-bit tiffs drop julia 0.5, view keyword arg, and 16-bit tiffs Aug 23, 2017
README.md Outdated
```

The optional `view` keyword argument is useful for reducing memory consumption
when loading large files.
Copy link
Member

@timholy timholy Aug 23, 2017

Choose a reason for hiding this comment

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

Maybe better to say "set view=true to reduce memory consumption when loading large files, possibly at some slight cost in terms of performance of future operations."

Copy link
Member Author

Choose a reason for hiding this comment

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

done. is there any way to add a doc string for sth like this? i tried adding one in src/ImageMagick.jl, but it didn't work after using FileIO.

Copy link
Member

@timholy timholy Aug 23, 2017

Choose a reason for hiding this comment

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

Interesting question, since FileIO is not meant to be extended. Maybe put it on ImageMagick.load? (In other words, does ?ImageMagick.load work as a way of adding a docstring for the load function in this specific package, without "contaminating" ?load?)

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, putting in on ImageMagick.load makes ?ImageMagick.load work. but given that one (or something) must do a using ImageMagick and that docstring is not displayed with ?load, is it worth it?

Copy link
Member

Choose a reason for hiding this comment

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

Certainly not necessary, but in the absence of a better idea I'm not sure where to aggregate the IM-specific extensions. We also have fps and perhaps other kwargs.

Copy link
Member

Choose a reason for hiding this comment

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

Not that you should feel responsible for doing this, simply asking the question is useful.

@timholy timholy merged commit 1a12ec2 into JuliaIO:master Aug 24, 2017
@timholy
Copy link
Member

timholy commented Aug 24, 2017

Thanks again!

@bjarthur bjarthur deleted the bja/view16 branch August 24, 2017 11:07
@bjarthur
Copy link
Member Author

i was planning to add a docstring this morning, but guess will leave it for another time. thanks!

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.

PermutedDimsArray instead of permutedims ?
2 participants