Draw mask callback #999

Dokholyan · 2020-11-13T16:56:04Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contribution guide?
Did you check the code style? catalyst-make-codestyle && catalyst-check-codestyle (pip install -U catalyst-codestyle).
Did you make sure to update the docs? We use Google format for all the methods and classes.
Did you check the docs with make check-docs?
Did you write any new necessary tests?
Did you check that your code passes the unit tests pytest . ?
Did you add your new functionality to the docs?
Did you update the CHANGELOG?

Description

Related Issue

Type of Change

Examples / docs / tutorials / contributors update
Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves an existing feature)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

PS

I know, that I could join slack for pull request discussion.

Dokholyan · 2020-11-14T16:07:12Z

I get a fail with "ModuleNotFoundError: No module named 'skimage'" , but I see it in the requirements. So I don't understand a problem

pep8speaks · 2020-11-16T16:26:36Z

Hello @Dokholyan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-12-10 18:03:57 UTC

bagxi · 2020-11-17T14:57:29Z

catalyst/contrib/callbacks/draw_masks_callback.py

+        elif activation == "Softmax2d":
+            self.activation = torch.nn.Softmax2d()
+        else:
+            self.activation = lambda x: x


How you feel about replacing lambda with torch.nn.Identity?

bagxi · 2020-11-17T14:59:44Z

catalyst/contrib/callbacks/draw_masks_callback.py

+    def _draw_masks(
+        self,
+        writer: SummaryWriter,
+        image_over_predicted_mask: np.array,


Please use np.ndarray

bagxi · 2020-11-17T15:00:57Z

catalyst/contrib/callbacks/draw_masks_callback.py

+        self,
+        writer: SummaryWriter,
+        image_over_predicted_mask: np.array,
+        image_over_gt_mask: Optional[np.array],


Is it possible to set the default value to None?

bagxi · 2020-11-17T15:03:41Z

catalyst/contrib/callbacks/draw_masks_callback.py

+        pred_mask_key: str,
+        image_key: Optional[str] = None,
+        gt_mask_key: Optional[str] = None,
+        mask2show: Optional[List[int]] = None,


tuples can also be used, so it is better to use Optional[Iterable[int]] here

bagxi · 2020-11-17T15:05:09Z

catalyst/contrib/callbacks/draw_masks_callback.py

+        if self.step % self.summary_step == 0:
+            pred_mask = runner.output[self.pred_mask_key][0]
+            pred_mask = self.activation(pred_mask)
+            pred_mask = pred_mask.cpu().detach().numpy()


Could you please use utils.detach?

Pull request has been modified.

Scitator

could you please

add your callback to the docs here https://github.com/catalyst-team/catalyst/blob/master/docs/api/contrib.rst
add changelog
merge correctly with master
add tests?

Pull request has been modified.

Dokholyan · 2020-12-05T16:50:46Z

@Scitator

could you please

add your callback to the docs here https://github.com/catalyst-team/catalyst/blob/master/docs/api/contrib.rst

add changelog

merge correctly with master

add tests?

Maybe it is better to add Callback here https://github.com/catalyst-team/catalyst/blob/master/docs/api/callbacks.rst ?
I am sorry, I don't understand
Ok, It is my fail
4)What test you would like to add?)

Scitator · 2020-12-06T07:01:05Z

yup, you are right
just follow the steps in "Before submitting" in the head of this PR
we have such segmentation test - https://github.com/catalyst-team/catalyst/blob/master/tests/_tests_cv_segmentation/config.yml, you could try to add your own callback there

Scitator · 2020-12-07T06:14:43Z

CHANGELOG.md

@@ -10,7 +10,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 ### Added

 - ([#1002](https://github.com/catalyst-team/catalyst/pull/1002))
-    - a few docs 
+- DrawMasksCallback ([#999](https://github.com/catalyst-team/catalyst/pull/999))
+- a few docs 


wrong ordering :)

Scitator · 2020-12-07T06:19:38Z

catalyst/contrib/callbacks/draw_masks_callback.py

+        pred_mask_key: str,
+        image_key: Optional[str] = None,
+        gt_mask_key: Optional[str] = None,


is it correct, that

pred_mask_key comes from runner.output

image_key comes from runner.input

gt_mask_key comes from runner.input
?

could we rename them into

output_key (or maybe even output_logits_key)

input_image_key

input_mask_key
?

what do you think about it?

Yes it is correct. predicted mask(logits) we get from "runner.output", image(input) and gt_mask(target) we get from "runner.input". The names are all debatable, if you want other names, I can change them.

could you please rename them?
it would be great if all our callbacks will follow the same naming

Scitator · 2020-12-07T06:21:59Z

catalyst/contrib/callbacks/draw_masks_callback.py

+            log_name: logging name. If you use several such "callbacks", they
+            must have different logging names


why do you need it? by default, we usually have tensorboard logger already for each dataset
could you reuse it?
here is an example, https://github.com/catalyst-team/catalyst/blob/master/catalyst/contrib/callbacks/confusion_matrix_logger.py#L41

I think that it is better to save images separately. Since they weigh a lot more than everything else, and sometimes there is a desire to remove them. Secondly, the log-name is the name in the Tensorboard, as a prefix for metrics

Pull request has been modified.

mergify · 2020-12-09T06:59:09Z

This pull request is now in conflicts. @Dokholyan, could you fix it? 🙏

Scitator · 2020-12-09T10:23:33Z

catalyst/contrib/callbacks/draw_masks_callback.py

+        pred_mask_key: str,
+        image_key: Optional[str] = None,
+        gt_mask_key: Optional[str] = None,


could you please rename them?
it would be great if all our callbacks will follow the same naming

Pull request has been modified.

smivv · 2020-12-12T18:40:01Z

Hello guys,

What do you think about splitting this callback into two different ones:

Mask plotter on the image (gets image + mask + gt_mask, plot them together and put in another output field).
Image logger to Tensorboard/Wandb/etc (gets whatever image you want and send it to log).

Currently I am starting to use Wandb and the part which corresponds to the image overlaying with mask seems to me quite useful, but without Tensorboard logging.

I consider this could be huge improvement for the granularity.

Dokholyan and others added 17 commits October 7, 2020 18:49

add DynamicBalanceClassSampler

ffb45ff

add DynamicBalanceClassSampler: add usage example

2146b89

add DynamicBalanceClassSampler: add tests

93a9d92

Update catalyst/data/tests/test_sampler.py

8573676

Update catalyst/data/tests/test_sampler.py

f4b21ae

add DynamicBalanceClassSampler: debag tests

a12a05c

update sampler: add mode

79332e1

add example notebook

ef33956

Merge remote-tracking branch 'original_C/master'

2ad65c6

sampler: fixes

d61fc8f

samler: docs

2be40b3

Merge remote-tracking branch 'original_C/master'

594328f

DynamicBalanceClassSampler: fixes

7c6a68e

change import order

f5dafe4

change import order

070a4ad

Merge remote-tracking branch 'original_C/master'

f578242

add draw_masks_callback

9c7a232

Dokholyan requested review from bagxi, ditwoo and Scitator as code owners November 13, 2020 16:56

fix legacy

5a05168

fix import

1da81bc

Dokholyan added 2 commits November 16, 2020 19:35

Merge remote-tracking branch 'original_C/master' into draw_mask_callback

644dcc8

fix import

2ec640c

bagxi previously requested changes Nov 17, 2020

View reviewed changes

fixes + white background

35e4abf

fix codestyle

a8f57c9

merge master

d260787

Scitator previously requested changes Dec 5, 2020

View reviewed changes

fix catalyst import

f8ba38a

Scitator added enhancement New feature or request WIP Pull request is under construction labels Dec 6, 2020

Dokholyan added 5 commits December 6, 2020 22:21

add draw_masks_callback

752d114

fix init

54103f3

add draw_casks callback to docs

182d921

add draw_masks_callack to pipeline

ad5d39a

Merge remote-tracking branch 'original_C/master' into draw_mask_callback

186f0a7

Scitator previously requested changes Dec 7, 2020

View reviewed changes

bagxi approved these changes Dec 7, 2020

View reviewed changes

bagxi previously approved these changes Dec 8, 2020

View reviewed changes

fix changelog

57d9ba1

merge master

5c3762e

Scitator previously requested changes Dec 9, 2020

View reviewed changes

Dokholyan added 2 commits December 10, 2020 11:04

rename keys

2a9c718

Merge remote-tracking branch 'original_C/master' into draw_mask_callback

8344a2e

Dokholyan added 3 commits December 10, 2020 20:29

fix keys

ef61b4a

Merge remote-tracking branch 'original_C/master' into draw_mask_callback

8993d90

fix activation keys

c288d50

Scitator merged commit 1aeabb7 into catalyst-team:master Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draw mask callback #999

Draw mask callback #999

		log_name: logging name. If you use several such "callbacks", they
		must have different logging names

Draw mask callback #999

Draw mask callback #999

Conversation

Before submitting

Description

Related Issue

Type of Change

PR review

Comment last updated at 2020-12-10 18:03:57 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment