Move `Defaults to` to end of arg docstring and standardise values by SamuelMarks · Pull Request #1848 · keras-team/keras-cv

SamuelMarks · 2023-05-31T15:16:57Z

What does this PR do?

Hey it's that annoying fellow again, the one from keras-team/keras#17748

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

ianstenbit

Thanks for the PR!

I chatted with the team and I think we're generally happy about this change -- thanks for the diligence here! I left one comment about structure, and then I'll dig through every little change once you've updated that 😄

benchmarks/vectorized_channel_shuffle.py

# Conflicts: # keras_cv/models/backbones/mobilenet_v3/mobilenet_v3_backbone.py # keras_cv/models/backbones/resnet_v1/resnet_v1_backbone.py

…resnet_v1_backbone.py] Resolve conflicts ; [keras_cv/models/backbones/resnet_v1/resnet_v1_{aliases,backbone}.py] Re-apply defaults-to after merge of master

SamuelMarks · 2023-06-30T02:02:33Z

3 weeks has gone by, @ianstenbit did you need me to merge master and redo this?

ianstenbit

Reviewed up through layers/preprocessing. I think may of the comments are repeats that will apply through the rest of the PR.

My apologies for the latency on this -- it's been a very busy few weeks and PR review has been slower.

benchmarks/vectorized_jittered_resize.py

benchmarks/vectorized_random_brightness.py

ianstenbit · 2023-06-30T02:21:59Z

examples/training/classification/imagenet/basic_training.py

 """

-# Try to detect an available TPU. If none is present, defaults to
+# Try to detect an available TPU. If none is present. Defaults to


Let's undo this change for all the comments in examples.

ianstenbit · 2023-06-30T02:22:18Z

keras_cv/datasets/imagenet/load.py

-        img_size: the size to resize the images to, defaults to None, indicating
-            that images should not be resized.
+            epoch. Defaults to `False`.
+        img_size: the size to resize the images to, when None, this indicates


when None

Makes sense to me:

the size to resize the images to, when None, this indicates that images should not be resized. Defaults to None.

Sorry I just meant use backticks for None. GitHub is rendering my backticks which makes these comments less clear 😓

ianstenbit · 2023-06-30T02:22:53Z

keras_cv/datasets/pascal_voc/segmentation.py

            used to download the data file, and unzip. It will be used as a
-            cache directory. Defaults to None, and `~/.keras/pascal_voc_2012`
-            will be used.
+            cache directory. When `None`: `~/.keras/pascal_voc_2012`


Maybe best to just say defaults to ~/.keras/pascal_voc_2012?

But it doesn't default to that value? - It computes to it sure, but defaults to None.

keras_cv/layers/preprocessing/random_apply.py

keras_cv/layers/preprocessing/random_brightness.py

keras_cv/layers/preprocessing/random_channel_shift.py

keras_cv/layers/preprocessing/random_crop_and_resize.py

keras_cv/layers/preprocessing/resizing.py

ianstenbit · 2023-06-30T02:29:28Z

3 weeks has gone by, @ianstenbit did you need me to merge master and redo this?

I think the merge conflicts shouldn't be too bad if you want to merge from master.

I left some comments on the first ~half, but it looks like there are some fixes that will need to be made through the rest as well. Thanks for the PR and my apologies for the review delay!

# Conflicts: # keras_cv/bounding_box/converters.py # keras_cv/models/backbones/csp_darknet/csp_darknet_backbone.py # keras_cv/models/backbones/efficientnet_v2/efficientnet_v2_backbone.py # keras_cv/models/backbones/resnet_v1/resnet_v1_backbone.py # keras_cv/models/backbones/resnet_v2/resnet_v2_backbone.py # keras_cv/models/legacy/densenet.py # keras_cv/models/legacy/segmentation/deeplab.py

SamuelMarks · 2023-07-25T21:46:36Z

@ianstenbit Merged in latest master and applied the defaults to changes to more places. It should be ready to go now; merge quick before master deviates!

ianstenbit

LGTM -- thank you!

Left a bunch of comments about tiny changes, but once those are in I can merge this. If you're able to make the changes today I can hold off on merging any other PRs to avoid merge conflicts.

Thanks again for the big cleanup!

ianstenbit · 2023-07-26T14:45:16Z

benchmarks/vectorized_jittered_resize.py

+            image. Defaults to `target_size`.
        bounding_box_format: The format of bounding boxes of input boxes.
            Refer to
-            https://github.com/keras-team/keras-cv/blob/master/keras_cv/bounding_box/converters.py


why drop the github.com prefix?

Will GitHub automagically render this link for us in the UI? That would be cool

@ianstenbit Oh actually I think it was flake8 picking up a long line

benchmarks/vectorized_random_zoom.py

examples/training/object_detection/pascal_voc/faster_rcnn.py

examples/training/object_detection/pascal_voc/retinanet.py

examples/training/semantic_segmentation/pascal_voc/basic_training.py

keras_cv/models/legacy/efficientnet_v1.py

keras_cv/models/legacy/vgg16.py

keras_cv/models/legacy/vgg19.py

keras_cv/models/legacy/vit.py

keras_cv/utils/train.py

@ianstenbit

… docstrings ; resolve nits from @ianstenbit

SamuelMarks · 2023-07-27T02:15:05Z

@ianstenbit Done

ianstenbit · 2023-07-27T18:35:01Z

@ianstenbit Done

It looks like MobileNet tests are failing -- I'd recommend reverting the change in the MobileNet backbone and merging from master

…ted 1 path from 3384a60

SamuelMarks · 2023-07-27T20:18:08Z

@ianstenbit I'm confused, what is it you want me to do?

git checkout master
git pull origin master # origin is keras-team/keras-cv
git checkout defaults-to
git merge master
git checkout master keras_cv/models/backbones/mobilenet_v3/mobilenet_v3_backbone_test.py
git checkout master keras_cv/models/backbones/mobilenet_v3/mobilenet_v3_aliases.py
git commit -S -a

ianstenbit · 2023-07-27T20:28:50Z

Merge branch 'master' into defaults-to

yeah that seems correct. Not clear what is causing this test failure, but I'd try reverting the changes to MobileNet (See my other comment)

SamuelMarks · 2023-07-28T19:42:48Z

Weird, isn't that what my checkout commands do? - Maybe it's another file causing the issue?

ianstenbit · 2023-07-28T21:41:31Z

Weird, isn't that what my checkout commands do? - Maybe it's another file causing the issue?

It could be another file. But checkout doesn't revert the changes that you've already committed, only staged / uncommitted changes by default

# Conflicts: # keras_cv/models/legacy/efficientnet_lite.py # keras_cv/models/legacy/efficientnet_v1.py

SamuelMarks · 2023-08-20T15:33:27Z

@ianstenbit Ok I've merged in master again. If you like, I can split this PR up into many small ones.

ianstenbit · 2023-08-21T15:42:35Z

@ianstenbit Ok I've merged in master again. If you like, I can split this PR up into many small ones.

It looks like the CI is still failing -- I think it's probably still the MobileNet issue from before but please take a look. Thanks!

SamuelMarks added 2 commits May 31, 2023 11:15

Move Defaults to to end of arg docstring and standardise values

c5b31ea

Resolve E501 lint errors

9ae3a40

ianstenbit suggested changes Jun 7, 2023

View reviewed changes

benchmarks/vectorized_channel_shuffle.py Outdated Show resolved Hide resolved

SamuelMarks added 3 commits June 7, 2023 17:45

[*.py] s/, defaults to/. Defaults to/g

b4c5fb7

Merge branch 'master' into defaults-to

3a6ffc8

# Conflicts: # keras_cv/models/backbones/mobilenet_v3/mobilenet_v3_backbone.py # keras_cv/models/backbones/resnet_v1/resnet_v1_backbone.py

[keras_cv/mobilenet_v3/mobilenet_v3_backbone.py,backbones/resnet_v1/…

9cc9478

…resnet_v1_backbone.py] Resolve conflicts ; [keras_cv/models/backbones/resnet_v1/resnet_v1_{aliases,backbone}.py] Re-apply defaults-to after merge of master

ianstenbit suggested changes Jun 30, 2023

View reviewed changes

SamuelMarks added 4 commits July 25, 2023 16:16

[*.py] Move Defaults to to end of arg docstring and standardise values

f515646

[*.py] Move Defaults to to end of arg docstring and standardise values

92a45c7

[benchmarks/vectorized_jittered_resize.py] Address nit from @ianstenbit

3ba1d76

ianstenbit approved these changes Jul 26, 2023

View reviewed changes

[*.py] Backtick quote None, True, False, and string literals in…

38731bf

… docstrings ; resolve nits from @ianstenbit

SamuelMarks added 2 commits July 27, 2023 16:13

Merge branch 'master' into defaults-to

1b6678d

[keras_cv/models/backbones/mobilenet_v3/mobilenet_v3_aliases.py] Upda…

a45e15b

…ted 1 path from 3384a60

SamuelMarks added 3 commits August 20, 2023 11:26

Merge branch 'master' into defaults-to

4ed54cf

# Conflicts: # keras_cv/models/legacy/efficientnet_lite.py # keras_cv/models/legacy/efficientnet_v1.py

Merge remote-tracking branch 'SamuelMarks/defaults-to' into defaults-to

9ce9fc1

# Conflicts: # keras_cv/models/legacy/efficientnet_lite.py # keras_cv/models/legacy/efficientnet_v1.py

[*.py] Move Defaults to to end of arg docstring and standardise values

201819d

ianstenbit mentioned this pull request Aug 21, 2023

[DeepVision Port] SegFormer and Mix-Transformers #1946

Merged

4 tasks

Conversation

SamuelMarks commented May 31, 2023

What does this PR do?

Before submitting

Who can review?

Uh oh!

ianstenbit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SamuelMarks commented Jun 30, 2023

Uh oh!

ianstenbit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ianstenbit commented Jun 30, 2023

Uh oh!

SamuelMarks commented Jul 25, 2023

Uh oh!

ianstenbit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SamuelMarks commented Jul 27, 2023

Uh oh!

ianstenbit commented Jul 27, 2023

Uh oh!

SamuelMarks commented Jul 27, 2023

Uh oh!

ianstenbit commented Jul 27, 2023

Uh oh!

SamuelMarks commented Jul 28, 2023

Uh oh!

ianstenbit commented Jul 28, 2023

Uh oh!

SamuelMarks commented Aug 20, 2023

Uh oh!

ianstenbit commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants