Closed RobertImbrie closed 3 years ago
Merging #686 (35e3043) into main (391cf34) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## main #686 +/- ##
=======================================
Coverage 96.85% 96.85%
=======================================
Files 63 63
Lines 9098 9098
=======================================
Hits 8812 8812
Misses 286 286
Impacted Files | Coverage Δ | |
---|---|---|
skll/data/readers.py | 90.13% <ø> (ø) |
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 391cf34...35e3043. Read the comment docs.
@RobertImbrie please take a look at my pushed changes and see if they make sense.
@desilinguist Hi! Apologies for the slow response. It does look good, with one exception! On line 754, you change
replace_blanks_with : value, ``dict``, or ``None``, default=None
to
replace_blanks_with : value, ``dict``, or ``None``, default=``None``
In both sklearn and the previous pushes, the default=ExampleVariable
section of the parameter header typically doesn't have quotes.
On a related note, did you have any input on using 'value' vs. Number
? I included a question about it in my initial comment.
@RobertImbrie good catch, please feel free to fix that quote issue. And yes, number instead of value seems like a better way to describe it! Please go ahead an make that change as well.
@desilinguist Changes have been pushed!
skll.FeatureSet
andskll.Reader
toskll.data.FeatureSet
andskll.data.Reader
replace_blanks_with
parameter descriptions to improve legibility.Notes: Before the pull request is approved, I'd like to note that both instances of the
replace_blanks_with
parameter include a 'value' option with unspecified type, shown here:Later, in the parameter description, it says that this value must be numeric:
If that's the case, shouldn't we refer to it as
Number
instead of 'value'?