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 memmap? #80

Closed
timholy opened this issue May 25, 2022 · 7 comments · Fixed by #82
Closed

Rename memmap? #80

timholy opened this issue May 25, 2022 · 7 comments · Fixed by #82
Milestone

Comments

@timholy
Copy link
Contributor

timholy commented May 25, 2022

memmap is used for incremental writing. However, as pointed out in #79,it's not actually using Mmap.mmap and perhaps should be renamed. What about empty_tiff?

Alternatively, you can use (warning: untested)

img = open(filename, "w") do io
    Mmap.mmap(io, Gray{N0f16}, sz3)
end
for i = 1:n
    img[:,:,i] = create_slice_data(i, sz2)   # this will write to the file
end

This should work at least on Linux despite apparently closing the file before values are assigned; Linux will defer the close until img gets garbage-collected. I am not sure about other platforms.

Of course, that doesn't create a TIFF. You'd need to do some work to write the header and set up empty chunks for the slice planes, essentially as MmappedTIFF does in #79. Basically, you mmap the entire file-buffer and then assign slices to correctly-placed views within the full buffer. Of course, if it's possible to pack all slice planes densely, then just a single view will suffice for an entire 3d array.

This may be more of a pain than any benefit you'd derive from it, so just renaming memmap seems expeditious.

@tlnagy
Copy link
Owner

tlnagy commented May 25, 2022

In light of #79, it would make sense to rename the memmap function. I don't love the empty_tiff name since it's not immediately apparent to me that it is on-disk instead of in memory. I would prefer

  • TiffImages.incremental
  • TiffImages.ondisk
  • Something else?

Longer term I think it would be nice to add push! support to MmappedTIFF and unified all disk ops under that type. I'm fine with throwing a deprecation warning for memmap in v0.6.0 and then removing it in the next minor release.

@timholy
Copy link
Contributor Author

timholy commented May 25, 2022

I'd be fine with either of those names.

push! support to MmappedTIFF would be possible but it seems likely to have constraints: I think that once the file is mmapped its size is fixed. I'm not entirely sure, though.

So push! could work, but we'd have to know the final size at the beginning and keep track of the "existing size" to know where to insert new data.

@tlnagy
Copy link
Owner

tlnagy commented May 25, 2022

Actually, the better solution is likely something like

img = empty(LazyTIFF, T, "filepath.tif")

where LazyTIFF is DiskTaggedImage renamed. I think that would be a more Julian approach.

What do you think?

@timholy
Copy link
Contributor Author

timholy commented May 25, 2022

That works for me!

@tlnagy tlnagy added this to the v0.6.0 milestone May 26, 2022
@tlnagy
Copy link
Owner

tlnagy commented May 26, 2022

Throwing the memmap deprecation and introducing the new empty syntax should probably happen before I tag v0.6.0 for release and then memmap can be removed in v0.7.0

@timholy
Copy link
Contributor Author

timholy commented Jun 2, 2022

I think you'll have to do the deprecation simultaneously with the 0.6.0 release. LazyBufferedTIFF will only exist in 0.6 and higher, and I don't think you really want to direct people to use empty(DiskTaggedImage, ...) for a single point release?

EDIT: or maybe I misunderstood you. I thought you were planning a 0.5.6 release in which memmap is deprecated, but perhaps not.

@tlnagy
Copy link
Owner

tlnagy commented Jun 2, 2022

No, you understood me properly. I'm about to submit the PR for the new empty function plus deps, just have to actually push it 😅

I'll tag the v0.6.0 release then.

tlnagy added a commit that referenced this issue Jun 2, 2022
@tlnagy tlnagy closed this as completed in #82 Jun 2, 2022
tlnagy added a commit that referenced this issue Jun 2, 2022
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 a pull request may close this issue.

2 participants