Skip to content
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

Update docstrings of basic.jl and conv.jl #1978

Merged
merged 21 commits into from
Jun 27, 2022

Conversation

Saransh-cpp
Copy link
Member

Hi everyone! Super excited to work on Flux's documentation as a JSoC student! This PR begins the first phase of my work, mainly auditing the documentation, fixing the outdated documentation, adding missing docstrings, and adding doctests to every docstring.

Summarising this PR, I have added missing docstrings, updated the existing ones, and added some doctests in the following files -

  • basics.jl
  • conv.jl

There are some minor changes in normalise.jl but I will be taking that complete file with some other files in the next PR. Or I can update all the files in this PR altogether, whichever method is easier to review :)

cc: @DhairyaLGandhi and all other community members!

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable

Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, my comments below:

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/normalise.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Show resolved Hide resolved
src/layers/normalise.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.10%. Comparing base (0b01b77) to head (297b822).
Report is 491 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1978   +/-   ##
=======================================
  Coverage   87.10%   87.10%           
=======================================
  Files          20       20           
  Lines        1528     1528           
=======================================
  Hits         1331     1331           
  Misses        197      197           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
@Saransh-cpp Saransh-cpp changed the title Add and fix some docstrings Update docstrings of basic.jl and conv.jl Jun 7, 2022
src/layers/conv.jl Outdated Show resolved Hide resolved
@Saransh-cpp
Copy link
Member Author

Moved the normalisation layers to #1995

src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this looks to be in a good place. I'm going to give it another day or so and then merge :)

@Saransh-cpp
Copy link
Member Author

Saransh-cpp commented Jun 27, 2022

Thank you for the reviews! Is this ready to be merged?

@ToucheSir ToucheSir merged commit d26ebdb into FluxML:master Jun 27, 2022
@ToucheSir
Copy link
Member

Yes it is, sorry it fell through the cracks!

@Saransh-cpp Saransh-cpp deleted the some-docstrings branch June 27, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants