search

fastaidocsprint / fastai

Documentation Sprint for the fastai deep learning library
http://fastaidocsprint.github.io/fastai
Apache License 2.0
15 stars 17 forks source link

document subplots and helpers in vision.data #69

Closed ChrisRichardMiles closed 2 years ago

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

ChrisRichardMiles commented 2 years ago

I didn't see a dropdown menu to request reviewers. Sorry this is my first time. In https://docs.fast.ai/vision.data.html#BBoxBlock I documented get_grid, clip_remove_empty, and bb_pad. since get_grid delegated to subplots in https://docs.fast.ai/torch_core.html, I also documented that.

ChrisRichardMiles commented 2 years ago

@muellerzr @warner-benjamin Ok I have included the types and return types and deleted extra spaces. I did not include return types in clip_remove_empty' orbb_pad` because they return the same types as the input types as per the style guide.

muellerzr commented 2 years ago

LG2M, awaiting final review from @warner-benjamin

ChrisRichardMiles commented 2 years ago

LG2M, awaiting final review from @warner-benjamin

But wait, it is not passing the CI. Sorry I did not see this. There was a problem with my tuple typing. I needed to replace tuple[..., ...] with typing.Tuple[..., ...]. I am fixing that now

Done It seems it doesn't do the CI checks on here until a moderator checks it, so I will make sure to test the notebooks before I push in the future.

ChrisRichardMiles commented 2 years ago

Ok I have removed typing library and added document for return types.

ChrisRichardMiles commented 2 years ago

@ChrisRichardMiles I added a commit removing a local env setup error message from both notebooks. Please don't check those messages in.

Everything else looks good and ready to merge. Thanks!

@warner-benjamin Thank you very much for catching that and fixing it. I didn't notice it. I will look for stuff like that in the future.