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
separate wrapper function #131
Conversation
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 think the consumer itself is still using multiplex_postprocess
instead of the new value.
It's the only one that's different from the defaults in deepcell_toolbox.
Should we move all of the defaults to here?
…On Mon, Sep 21, 2020 at 12:22 PM willgraf ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I think the consumer itself is still using multiplex_postprocess instead
of the new value.
------------------------------
In redis_consumer/processing.py
<#131 (comment)>
:
> + if whole_cell_kwargs is None:
+ whole_cell_kwargs = {}
+ whole_cell_kwargs['radius'] = 5
+
+ if nuclear_kwargs is None:
+ nuclear_kwargs = {}
+ nuclear_kwargs['radius'] = 5
Is radius the only one we want to default?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#131 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADJB47NPTWVYYQLSJFR4VYDSG6RVZANCNFSM4RSO57HA>
.
|
Actually nevermind, I'm going to update them with the newest
post-processing values, so I'll update all of them, and set the defaults
here. Will make things easier to update moving forward.
On Mon, Sep 21, 2020 at 1:44 PM Noah Greenwald <noahfgreenwald@gmail.com>
wrote:
… It's the only one that's different from the defaults in deepcell_toolbox.
Should we move all of the defaults to here?
On Mon, Sep 21, 2020 at 12:22 PM willgraf ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
>
> I think the consumer itself is still using multiplex_postprocess instead
> of the new value.
> ------------------------------
>
> In redis_consumer/processing.py
> <#131 (comment)>
> :
>
> > + if whole_cell_kwargs is None:
> + whole_cell_kwargs = {}
> + whole_cell_kwargs['radius'] = 5
> +
> + if nuclear_kwargs is None:
> + nuclear_kwargs = {}
> + nuclear_kwargs['radius'] = 5
>
> Is radius the only one we want to default?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#131 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ADJB47NPTWVYYQLSJFR4VYDSG6RVZANCNFSM4RSO57HA>
> .
>
|
Yeah I think it makes sense to add all the values here so they're easier to tweak moving forward. |
Okay, I updated it so there are defaults for all of them. Currently, the consumer post-processing function doesn't take additional arguments. This doesn't matter right now, since there's no way for users to control any of the post-processing parameters. Eventually, we might want to add that option to the ImageJ app or deepcell.org. When then happens, we'd need to add a way for the base post-processing class to handle additional arguments. However, since right now this doesn't exist, I set it up so that the 'defaults' are always used. |
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 looks good, but I'd like to test the consumers out before we merge it.
Yup, deepcell.org and ImageJ both gave good looking results |
This provides a way for us to modify the post-processing params that are sent to the consumer.
Let me know if you think it would be better to have this somewhere else before I start moving forward