Repository navigation
Address remaining concerns from the 2nd DataFrame API Review - #2861
Conversation
| return df.Reverse<#=method.MethodName#>(value, inPlace: false); | ||
| } | ||
|
|
||
| <# // There seem to be very few use cases for an API such as df.And/Or/Xor(true). #> |
There was a problem hiding this comment.
Maybe replace very few with no use cases
| return df.ReverseAdd(value); | ||
| } | ||
|
|
||
| public static DataFrame operator +(DataFrame df, char value) |
There was a problem hiding this comment.
For conversational sake - could this be useful for appending a character to string columns? Or would they just use a full string in that case?
There was a problem hiding this comment.
2 things:
- This operator is on the DataFrame itself. So it'd be useful only if we had a DataFrame composed of only char columns. Even then, the current behavior is not great: 'a' + 'a' results in whatever character
194is. - Assuming you mean the same operator but defined on the
DataFrameColumntype, then no, it wouldn't be useful. We'd just use the fullStringDataFrameColumnin that case. We don't have the ability to go from aPrimitiveDataFrameColumn<char>->StringDataFrameColumnyet.
| return df.ReverseModulo(value); | ||
| } | ||
|
|
||
| public static DataFrame operator &(DataFrame df, bool value) |
There was a problem hiding this comment.
Because it'd only be useful when we have a DataFrame composed of only Boolean columns(because &/|/^ ABoolValue don't make sense on any other column). I think the feeling in the API review was that it didn't make sense to keep this API for a corner case. The operators are still defined on the base DataFrameColumn type though, so a statement like boolColumn & true is valid.
Should the summary be updated to not say "clip" anymore? https://docs.microsoft.com/en-us/dotnet/api/system.math.clamp?view=netcore-3.1 Uses the summary:
Refers to: src/Microsoft.Data.Analysis/DataFrame.cs:251 in 16dc6ea. [](commit_id = 16dc6ea, deletion_comment = False) |
| /// <param name="lower">Minimum value. All values below this threshold will be set to it</param> | ||
| /// <param name="upper">Maximum value. All values above this threshold will be set to it</param> | ||
| public DataFrame Clip<U>(U lower, U upper, bool inPlace = false) | ||
| public DataFrame Clamp<U>(U lower, U upper, bool inPlace = false) |
There was a problem hiding this comment.
Maybe lower => min and upper => max?
There was a problem hiding this comment.
Yup. Changed the parameter names in the Filter too
All of these changes are from the API review. Pretty straightforward