Skip to content

Add a doc template to SkipTakeFilter #3415

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

Closed
wants to merge 1 commit into from
Closed

Conversation

wschin
Copy link
Member

@wschin wschin commented Apr 18, 2019

Toward #3204.

@wschin wschin added the documentation Related to documentation of ML.NET label Apr 18, 2019
@wschin wschin self-assigned this Apr 18, 2019
@wschin wschin requested review from shmoradims and sfilipi April 18, 2019 22:03
@codecov
Copy link

codecov bot commented Apr 18, 2019

Codecov Report

Merging #3415 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3415      +/-   ##
==========================================
+ Coverage   72.69%   72.69%   +<.01%     
==========================================
  Files         807      807              
  Lines      145171   145171              
  Branches    16225    16225              
==========================================
+ Hits       105536   105539       +3     
+ Misses      35220    35219       -1     
+ Partials     4415     4413       -2
Flag Coverage Δ
#Debug 72.69% <ø> (ø) ⬆️
#production 68.23% <ø> (ø) ⬆️
#test 88.97% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.Data/Transforms/SkipTakeFilter.cs 88.33% <ø> (ø) ⬆️
...StandardTrainers/Standard/LinearModelParameters.cs 60.31% <0%> (+0.26%) ⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 85.11% <0%> (+0.4%) ⬆️

1 similar comment
@codecov
Copy link

codecov bot commented Apr 18, 2019

Codecov Report

Merging #3415 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3415      +/-   ##
==========================================
+ Coverage   72.69%   72.69%   +<.01%     
==========================================
  Files         807      807              
  Lines      145171   145171              
  Branches    16225    16225              
==========================================
+ Hits       105536   105539       +3     
+ Misses      35220    35219       -1     
+ Partials     4415     4413       -2
Flag Coverage Δ
#Debug 72.69% <ø> (ø) ⬆️
#production 68.23% <ø> (ø) ⬆️
#test 88.97% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.Data/Transforms/SkipTakeFilter.cs 88.33% <ø> (ø) ⬆️
...StandardTrainers/Standard/LinearModelParameters.cs 60.31% <0%> (+0.26%) ⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 85.11% <0%> (+0.4%) ⬆️

/// ]]>
/// </format>
/// </remarks>
/// <seealso cref="Microsoft.ML.DataOperationsCatalog.SkipRows(Microsoft.ML.IDataView,System.Int64)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

SkipRows [](start = 58, length = 8)

What's the point of all this if class for this summary is internal?

Copy link
Member

Choose a reason for hiding this comment

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

also, I'd leave the filters out, since they don't change the schema.


In reply to: 276867180 [](ancestors = 276867180)

Choose a reason for hiding this comment

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

We don't need this 'internal' change. Also filters are different from transforms and don't return an estimator. they return an IDV.

The actual public API is here:

public IDataView SkipRows(IDataView input, long count)

I see that Rogan has already polished them as part of the samples. I think they're in good shape. I'll close this PR. If you think otherwise, please reopen.


In reply to: 276907066 [](ancestors = 276907066,276867180)

@@ -26,8 +26,23 @@
namespace Microsoft.ML.Transforms
{
/// <summary>
/// Allows limiting input to a subset of row at an optional offset. Can be used to implement data paging.
/// Allows limiting input to a subset of row at an optional offset. It can either take first <see cref="_take"/> rows and ignore all others.
Copy link
Member

Choose a reason for hiding this comment

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

row [](start = 45, length = 3)

rows

@shmoradims
Copy link

Looks like this was already covered by Rogan as part of samples work. Please see my comment above for details. Closing this PR for now.

@shmoradims shmoradims closed this Apr 20, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Related to documentation of ML.NET
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants