toward codecs and shards #32
Conversation
* added a failing test
# Conflicts: # pom.xml
# Conflicts: # pom.xml
OmeNgffMetadataParser for NGFF v0.5
There was a problem hiding this comment.
During the code-review talk today, I noted down any comments I had on the demos and benchmarks. A lot of them are just minor comments on syntax, but there are 2 or 3 conceptual questions I had. Let me know if I need to clarify any of them.
Also, because it was the first file in the diff, I started doing a little review on N5DatasetDiscoverer, which I realized later might still be work in progress, so take this with a grain of salt.
| private final List<N5MetadataParser<?>> metadataParsers; | ||
| private final List<N5MetadataParser<?>> groupParsers; | ||
|
|
||
| private final Comparator<? super String> comparator; |
There was a problem hiding this comment.
The name could be a little more descriptive. What does it compare?
| * @param comparator | ||
| * optional string comparator | ||
| * @param filter | ||
| * the dataset filter |
There was a problem hiding this comment.
Why is comparator optional, but filter isn't, even though null is passed for that argument in the other constructors?
Looking at the code below, it seems that null is used internally to mark the absence of something, anyway.
| * @throws IOException | ||
| * the exception | ||
| */ | ||
| public static void parseMetadata(final N5Reader n5, final N5TreeNode node, |
There was a problem hiding this comment.
I'm not sure if I understand the intent of this method. Is it "I don't know which metadata standard this adheres to, so I try all of them"? In that case, this seems more like a discovery mechanism instead of a deterministic metadata parsing, i.e., it could fail, which is not really reflected in the method signature.
In particular, the docstring for the exception that is thrown could be a little more specific.
| } | ||
| } | ||
|
|
||
| public static boolean trim(final N5TreeNode node) { |
There was a problem hiding this comment.
There is no documentation for trim(N5TreeNode) and the one for trim(N5TreeNode, Consumer<N5TreeNode>) doesn't mention any details about the difference between the methods (the callback).
| return node.getMetadata() != null; | ||
| } | ||
|
|
||
| boolean ret = false; |
There was a problem hiding this comment.
I suggest naming booleans like a predicate, e.g., trimHasSucceeded or canBeTrimmed.
|
|
||
| int i = 0; | ||
| Iterator<long[]> it = shard.blockPositionIterator(); | ||
| while( it.hasNext()) { |
There was a problem hiding this comment.
| while( it.hasNext()) { | |
| while(it.hasNext()) { |
|
|
||
| VirtualShard<UnsignedByteType> shard = (VirtualShard<UnsignedByteType>) n5Writer.getShard(path, attrs, 0); | ||
| List<DataBlock<UnsignedByteType>> blks = shard.getBlocks(blockIndexes); | ||
| for( DataBlock<UnsignedByteType> blk : blks) |
There was a problem hiding this comment.
| for( DataBlock<UnsignedByteType> blk : blks) | |
| for(DataBlock<UnsignedByteType> blk : blks) |
|
|
||
| private void readShardWhole(Blackhole blackhole, String path) { | ||
|
|
||
| InMemoryShard<UnsignedByteType> shard; |
There was a problem hiding this comment.
Can shard be declared in the try block?
| for (DataBlock<UnsignedByteType> blk : shard.getBlocks(blockIndexes)) | ||
| blackhole.consume(blk); | ||
|
|
||
| } catch (IOException e) { } |
There was a problem hiding this comment.
| } catch (IOException e) { } | |
| } catch (IOException ignored) { } |
| } catch (final NoSuchFieldException e) { | ||
| e.printStackTrace(); | ||
| } catch (final SecurityException e) { | ||
| e.printStackTrace(); | ||
| } |
No description provided.