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
Add decoding functionality to Polaris #36
Add decoding functionality to Polaris #36
Changes from 57 commits
a506697
f9177f8
3d0f10b
9542980
155372b
da06afb
3993292
4241534
67ae54c
5c0db27
fed160a
bb0ecbd
b365a17
8ec7aea
1d3c759
7382e94
0aea80a
55465d4
4aa0675
d22e8a0
39351d5
2ed66b4
fa860fb
abbbd34
97cbec6
f9a7eac
86d3fc8
f65387a
3936ae8
a1260cc
153f0eb
a7fb152
c3fde7b
0027cfd
64e7286
b23cf0a
e7cf81e
ab3ea8e
9b1d37e
fbeb984
566a1b8
131d7f3
28978f9
f5974f3
18516aa
c3ce153
30b8c34
3412bd5
eedc015
8996c66
22afcb4
c28d635
cc140d2
d849e28
c2c4a80
5fc7775
2269b68
ee282cb
1192f04
0b6f592
3fedce5
b892320
4eca71f
497562b
35c3dd6
7d80d32
727592f
cbe6c8f
be92fbc
27f1d96
c8a4055
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.
tl;dr - this probably doesn't matter, but may break existing code. It's safe to ignore if you don't care about backward compatibility yet!
Technically, adding a kwarg to the front of a signature like this will break any existing code that uses position-based arguments. For example, if there is existing code where a
Polaris
object was instantiated like:this change in the signature will break that code, because now the first unnamed argument maps to
image_type
instead ofsegmentation_model
.If you don't have a ton of existing code that you're worried about breaking, you can go ahead and ignore this for now! Another solution would be to use keyword-only arguments (i.e. start this signature list with
*,
) which prevents users from creatingPolaris
objects with unnamed args. This may still break existing code, but it will fail loudly with a very specific message and a straightforward, future-proof fix.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'm not sure what the shape of
output_image
is expected to be, but it's worth double-checking (and eventually testing) that you are taking the maximum projection along the dimension you expect.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.
It's tough to say without knowing the shapes/types of all the inputs, but this looks like it might be creating a ragged array (i.e. where the shape is not a constant along one of the dimensions) which could have poor performance depending on the types of subsequent operations performed.
If this is the case, it might be worth switching to a dictionary or other mapping-type data structure.