Update VBuffer documentation #3136
Conversation
src/Microsoft.ML.DataView/VBuffer.cs
Outdated
| /// <param name="ivDst">The starting index of <paramref name="dst"/> at which to start copying</param> | ||
| /// <param name="defaultValue">The value to fill in for the implicit sparse entries. This is a potential exception to | ||
| /// general expectation of sparse <see cref="VBuffer{T}"/> that the implicit sparse entries have the default value | ||
| /// of <typeparamref name="T"/>.</param> | ||
| public void CopyTo(Span<T> dst, int ivDst, T defaultValue = default(T)) |
There was a problem hiding this comment.
ivDst [](start = 44, length = 5)
Looks like we still have some acronyms and whatever this is lurking around. Not sure if care enough to change the API for v1...
There was a problem hiding this comment.
Note that changing a parameter name after you ship an API is a breaking change. So it's now or never.
There was a problem hiding this comment.
Well... I was hoping to not make this one of the cherry picked PRs. Let me do this, I'll do the review comments in one commit, then do the changes to parameters in an isolated commit, and then we can decide what we want to do. /cc @shauheen
Adding label "shiproom-review" in light of this... sigh.
There was a problem hiding this comment.
OK, so, last commit titled Update parameter names to avoid abbreviations, e.g., src => source and suchlike. is up, I updated the parameter names in VBuffer. Names in VBufferEditor were fine. I also updated the documenation in the .md file, but I did not update the actual code that inspired the example since that is in the internal API. (But, the documentation uses the idea merely as a simple, easy to understand example, so I think that's fine.) I took some queues from things like Array.Copy, but without going to far as calling the parameters VBuffer<T> sourceBuffer since I felt that might be going a bit too far, I just named them things like source rather than go quasi-hungarian w.r.t. the names, so, just source, destination, etc., as I believe you also did in VBufferEditor @eerhardt (e.g., your Create methods take destination, not destinationBuffer).
There was a problem hiding this comment.
BTW on the same subject of renamings, there was one place @eerhardt pointed out where an index was being named slot for some reason (since I guess we conceptually separate the concepts of "columns" and "slots" in data view, but, for VBuffer specifically, this should have just been index).
Perhaps, it should mention how to represent multi-dimensional data, such as multi-color pixels? (either howto or mention the limitation.) #WontFix Refers to: docs/code/VBufferCareFeeding.md:9 in ce4618d. [](commit_id = ce4618d, deletion_comment = False) |
Hmmm. I agree that's worth mentioning, but that really has nothing to do with For that reason, would you mind if I saved this clarification to the point when I am working on updating the |
Sounds good. I also took the opportunity to reinforce the idea of implicit and explicit values. |
* Improve the XML documentation for VBuffer/VBufferEditor. * Update the "best practices" documentation to reflect recent changes.
|
Thanks @glebuk, I updated it, let me know if you're happy with it yet. |
Codecov Report
@@ Coverage Diff @@
## master #3136 +/- ##
==========================================
- Coverage 72.52% 72.51% -0.01%
==========================================
Files 808 808
Lines 144665 144665
Branches 16198 16198
==========================================
- Hits 104912 104906 -6
- Misses 35342 35348 +6
Partials 4411 4411
|
Codecov Report
@@ Coverage Diff @@
## master #3136 +/- ##
==========================================
+ Coverage 72.52% 72.52% +<.01%
==========================================
Files 808 808
Lines 144665 144740 +75
Branches 16198 16202 +4
==========================================
+ Hits 104912 104977 +65
- Misses 35342 35352 +10
Partials 4411 4411
|
* Improve VBuffer documentation. * Improve the XML documentation for VBuffer/VBufferEditor. * Update the "best practices" documentation to reflect recent changes. * Update parameter names to avoid abbreviations, e.g., src => source and suchlike.
ghost
locked as resolved and limited conversation to collaborators
Towards #3095, specifically about
VBuffers. Changes documentation to reflect the changes made by @eerhardt in his refactoring of #1580.