search

fastai / fastai2

Temporary home for fastai v2 while it's being developed
https://dev.fast.ai
Apache License 2.0
645 stars 234 forks source link

Added saturation and hue transforms #525

Closed marii-moe closed 4 years ago

marii-moe commented 4 years ago

Wanted to share some code to get some feedback.

Possible Issues:

  1. Preferred way to give credit to code referenced?
  2. Hue transform is not in logit scale, so it cannot be composed with lighting transforms. Hue doesn't have a maximum or minimum value. 0 is red, go far enough and wraps back around to red. https://en.wikipedia.org/wiki/HSL_and_HSV
  3. Adding new images caused the number of lines changed in the notebook to balloon, anyway to fix this?
  4. I'm still learning, so I am sure there is more.
review-notebook-app[bot] commented 4 years ago

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

jph00 commented 4 years ago

This is looking good! The main thing to consider is that with notebooks you're not just writing code, but you're also writing docs. So use plenty of markdown cells, with plenty of links, to explain what each function does, why it's useful, etc.

For refs/cites, for actual code you can just add comments. For refs to ideas, add a link in the markdown.

marii-moe commented 4 years ago

Okay, thank you! Is there a gold standard notebook for this level of markdown cells and links? I was trying to copy the level of documentation/tests in nbs/09_vision.augment.ipynb, but maybe it also needs more documentation?

jph00 commented 4 years ago

Yes it definitely needs more through much of the notebook. Here's a better example (at least, the start of it): http://dev.fast.ai/callback.hook#What-are-hooks?

Note that docstrings in fastai should always fit on one line, and not use triple quotes.

jph00 commented 4 years ago

I'm going to merge this as-is since I'm moving the repos today. Hope it's OK for your to do another PR to fix the minor issues mentioned.