Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ENH: Enable heatmaps when tiling on the fly #491
base: main
Are you sure you want to change the base?
ENH: Enable heatmaps when tiling on the fly #491
Changes from 23 commits
df92a48
a7754d3
6f29abe
ce04dab
be25cf6
8183fb6
1a93d3a
bbaa5fa
5c10885
585c893
7970037
792385e
85bd4ec
5281940
2ea8c94
a720a28
a996323
547889f
7550cae
e989b4f
73f0798
3418f47
73b044f
58c74ec
fec8f8e
87e693f
bf16cb0
b656599
6408a2e
aa3e6f1
4226ba3
4a97089
50a58c0
6dfa5d6
600dd06
33c52b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you think of using our
Box
class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on that. Tuples with 4 elements of the same type are just too easy to mix up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add doc string please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might this be cleaner using numpy arrays, perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dccastro , I like your suggestion above to use the
Box
class instead. Much cleaner than using a 4-tuple of ints, that's just calling for errors to happen.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this method need to mutate the input
metadata_dict
in-place, instead of returning a new dictionary?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. If needs to mutate in place, then the signature and name of the function should reflect that mutation. (
update_metadata
,-> None
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Minor] The
.value
shouldn't be necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a set operation would be easier to understand.
set(SlideKey.OFFSET, SlideKey....) <= set(batch.keys()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this be replaced by just adding
if key not in results: results[key] = []
in the loop below?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if all this metadata processing should be wrapped into a transform. Data is prefetched anyways it should be more efficient to apply it as a transform that is muti processed by the dataloader. Also from a design perspective, the model shouldn't be handling any metadata processing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why do we need to do that here? is that for the coordinates? Maybe turn it into numpy arrays in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function could do with a lot more documentation. A docstring would be great. And each of the
if
branches should have a clear description which case we are handling here, and where those cases arise.