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

Fix chunking issues in NDFileHDF5 #404

Merged
merged 18 commits into from
May 10, 2019
Merged

Fix chunking issues in NDFileHDF5 #404

merged 18 commits into from
May 10, 2019

Conversation

MarkRivers
Copy link
Member

This PR fixes the issues identified in #401:

  • User-defined chunking only supported 2-D arrays, plus the dimension for multiple arrays. This is insufficient, we need to be able to support N-dimensional arrays in a general manner.
  • Direct chunk write only worked with 1-D or 2-D arrays. This prevented it from working with RGB images, for example.
  • I have retained the existing NumColChunks (dimension 0), NumRowChunks (dimension 1), and added new records ChunkSize2, ChunkSize3, ... ChunkSize9 for dimensions 2-9. I also retained NumFramesChunks.
  • I modified the labels on the OPI for the NumColsChunks and NumRowsChunks records to be "Dim0 chunk size" and "Dim1 chunk size", and added "Dim2 chunk size" to this screen as well. I added a new More screen that shows all 12 chunking related records (10 dimensions, NumFramesChunks, ChunkSizeAuto).
  • The documentation said that

... setting the nColChunks and nRowChunks parameters to the X and Y frame size respectively,
should give a decent result. In fact if these parameters are configured by the user to the special value 0,
they will default to the dimensions of the incoming frames.

  • However, this was not really true. Setting to 0 only works once. If they are set to 0 and a 512x512 array is saved then the chunking will be set to 512x512. If the user then disables binning and saves a 1024x1024 array the chunking remains at 512x512, which is not desirable.
  • I added new ChunkSizeAuto bo/bi records to allow always setting the chunking to automatic, or disabling this and allowing manual chunking control. The automatic control does not affect NumFramesChunks.
  • I fixed a problem in NDFiileHDF5::calcNumFrames(), it was setting NumCapture to 1 if numExtraDims=0, which is incorrect. This caused the autosaved value to be replaced by 1 each time the IOC restarted.

@coveralls
Copy link

coveralls commented May 1, 2019

Coverage Status

Coverage increased (+0.08%) to 40.222% when pulling 9a4a67c on fix_chunking into 579581a on master.

@MarkRivers
Copy link
Member Author

Someone who uses extraDimensions should check that it is still working correctly after the chunking changes. The unit tests in pluginTests do some extra dimension tests, and they are passing OK, but someone should do a more real-world test.

@ulrikpedersen ulrikpedersen requested a review from ajgdls May 8, 2019 14:18
@ulrikpedersen
Copy link
Member

@ajgdls will take a look at this one

@ajgdls
Copy link
Contributor

ajgdls commented May 9, 2019

I've run a suite of system tests that check the multi-dimensional writes, positional placement of arrays within extra dimensions and chunking. They all pass except for two tests, which involve different chunking values when using extra dimensions. I believe these two do not fail as a result of your changes here @MarkRivers but instead fail due to an attempt to incorrectly write them with direct chunk write.

However I'll push the fix to this branch if you don't have any objections as it is related.

@MarkRivers
Copy link
Member Author

That sounds good to me.

MarkRivers and others added 4 commits May 9, 2019 11:14
…at the TIFF standard allows.

Allow use of TIFFTAG_LAST_ATTRIBUTE tag.  Previous it only allowed up to TIFFTAG_LAST_ATTRIBUTE-1.
Fixed error in definition of NUM_CUSTOM_TIFF_TAGS which could lead to access violation.
@ajgdls
Copy link
Contributor

ajgdls commented May 10, 2019

Hi @MarkRivers @GDYendell I have pushed a commit with the extra check to ensure direct chunk write isn't used if extra dimension chunk sizes are not 1. All of our system tests and the unit tests now pass. I would appreciate if you could take a look at this commit as the change was not quite as simple as I had anticipated.

@GDYendell
Copy link
Contributor

GDYendell commented May 10, 2019

It looks OK to me. Am I right in thinking this isn't supposed to pass the extra chunk dims through, it just checks they are 1 here and then blindly inserts 1s later when using direct chunk write?

@ajgdls
Copy link
Contributor

ajgdls commented May 10, 2019

I didn't alter any of the code that actually performs the writing (as that was already working as expected), so I guess the answer to your question is yes this is purely checking the values here to decide if we can direct chunk write or not.

@MarkRivers MarkRivers merged commit e7b09a1 into master May 10, 2019
@MarkRivers MarkRivers deleted the fix_chunking branch May 10, 2019 22:05
bsobhani pushed a commit to bsobhani/ADCore that referenced this pull request Apr 1, 2024
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