Skip to content

Conversation

@stas00
Copy link
Contributor

@stas00 stas00 commented Sep 3, 2020

  • fix docstring s/int/bool/
  • correct arg description
  • fix num_labels to match reality

@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #6932 into master will increase coverage by 1.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6932      +/-   ##
==========================================
+ Coverage   77.70%   79.53%   +1.83%     
==========================================
  Files         161      161              
  Lines       30119    30119              
==========================================
+ Hits        23403    23956     +553     
+ Misses       6716     6163     -553     
Impacted Files Coverage Δ
src/transformers/configuration_bart.py 94.00% <ø> (ø)
src/transformers/modeling_tf_albert.py 21.47% <0.00%> (-69.44%) ⬇️
src/transformers/tokenization_xlm.py 16.26% <0.00%> (-66.67%) ⬇️
src/transformers/modeling_tf_gpt2.py 71.84% <0.00%> (-23.17%) ⬇️
src/transformers/modeling_xlnet.py 60.81% <0.00%> (-22.62%) ⬇️
src/transformers/modeling_longformer.py 71.55% <0.00%> (-20.48%) ⬇️
src/transformers/data/data_collator.py 86.63% <0.00%> (-6.08%) ⬇️
src/transformers/modeling_tf_xlm.py 88.42% <0.00%> (-4.85%) ⬇️
src/transformers/file_utils.py 82.41% <0.00%> (-0.26%) ⬇️
src/transformers/modeling_bert.py 88.26% <0.00%> (-0.17%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e95d262...1c08fdb. Read the comment docs.

@stas00 stas00 changed the title [docstring] correct bool types [docstring] misc arg doc corrections Sep 3, 2020
@sgugger
Copy link
Collaborator

sgugger commented Sep 4, 2020

Thanks. Will in these fixes don't hesitate to replace the True or False by :obj:`True` and :obj:`False` for consistency (also don't hesitate to tag me on doc PR for quicker reviews :-) )

@sgugger sgugger merged commit c5d43a8 into huggingface:master Sep 4, 2020
@stas00
Copy link
Contributor Author

stas00 commented Sep 4, 2020

Understood!

Do we have a model of perhaps one largish module that we can use a reference for how the rest should be done? So that you polish the hell out of it, and then this will be the model to follow.

@stas00 stas00 deleted the patch-2 branch September 4, 2020 16:45
@sgugger
Copy link
Collaborator

sgugger commented Sep 4, 2020

tokenization_utils_base is a good example for instance, all other utils modules too. config_utils has an example of how to split parameters in several subgroups if you ever need a model of that. Rules are the usual sphinx-like and some more personal nits are:

  • not writing "defaults to :obj:`None`" for optional things that have default (it's implied)
  • using :obj:`foo` syntax for objects (like False, True, all strings) or mention to other arguments
    but not numbers (like 0, 1.0...)
  • using italics for optional

@stas00
Copy link
Contributor Author

stas00 commented Sep 4, 2020

Great tips on the model docs and the small specifics. I see some are already here:
https://github.com/huggingface/transformers/tree/master/docs#writing-documentation---specification
add the others too?

Loving params subgroup docs - it's very helpful. I'd organize the params in the function in the same groups too.

Thank you for sharing all these, @sgugger!

@sgugger
Copy link
Collaborator

sgugger commented Sep 4, 2020

Yes we could add those general rules to that section of the docs README. (I am unsure people actually read that so did not take the time to properly update it :-) )

@stas00
Copy link
Contributor Author

stas00 commented Sep 4, 2020

I didn't know it was there, but now that I do, I'd definitely per-use it - so yes, please update it!

@stas00
Copy link
Contributor Author

stas00 commented Sep 4, 2020

* not writing "defaults to :obj:`None`" for optional things that have default (it's implied)
grep -r "defaults to :obj:.None." src | wc -l
580

might be easy to replace in one swoop.

@sgugger
Copy link
Collaborator

sgugger commented Sep 4, 2020

Feel free to do it in one PR :-)

@stas00
Copy link
Contributor Author

stas00 commented Sep 4, 2020

Done: #6956

Zigur pushed a commit to Zigur/transformers that referenced this pull request Oct 26, 2020
* correct bool types

fix docstring s/int/bool/

* fix description

* fix num_labels to match reality
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.

2 participants