You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "sgilmore10 (via GitHub)" <gi...@apache.org> on 2023/06/07 17:47:55 UTC
[GitHub] [arrow] sgilmore10 opened a new pull request, #35977: GH-35693: [MATLAB] Add Valid as a name-value pair on the arrow.array.Array superclass constructor
sgilmore10 opened a new pull request, #35977:
URL: https://github.com/apache/arrow/pull/35977
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Users should be able to specify exactly which elements in a MATLAB array should be treated as `null` values. In a previous PR, we added the name-value pair `InferNulls` which users can use to turn off the automatic "null detection" of values that `ismissing` function returns true:
```MATLAB
>> matlabArray = [1 NaN 3];
% NaN is treated as null by default
>> arrowArray = arrow.array.Float64Array(matlabArray);
arrowArray =
[
1,
null,
3
]
% Don't treat NaN as a null value
>> arrowArray = arrow.array.Float64Array(matlabArray, InferNulls=false);
arrowArray =
[
1,
nan,
3
]
```
To provide more control, this PR adds a new name-value pair named `Valid`. This name-value pair will take precedence over `InferNulls` and allows users to specify which elements are valid using either a logical array or a list of indices:
```MATLAB
>> matlabArray = [1 NaN 3];
>> arrowArray = arrow.array.Float64Array(matlabArray, Valid=[true true false]);
arrowArray =
[
1,
nan,
null
]
>> arrowArray = arrow.array.Float64Array(matlabArray, Valid=[2 3]);
arrowArray =
[
null,
nan,
3
]
```
### What changes are included in this PR?
1. Added the `Valid` name-value pair to `arrow.array.Float64Array`.
### Are these changes tested?
1. Added a new test file called `tParseValidElements.m` to independently test the helper function `arrow.args.parseValidElements` used by `arrow.array.Float64Array`.
2. Added two integration tests in `tFloat64Array.m`.
### Are there any user-facing changes?
Yes, users can now pass the `Valid` name-value pair to `arrow.array.Float64Array`.
###Future Directions
1. Currently, only `arrow.array.Float64Array` supports the `InferNulls` and `Valid` name-value pairs. In a followup PR, we will add support for these name-value pairs to the rest of the numeric array classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35977:
URL: https://github.com/apache/arrow/pull/35977#issuecomment-1581411377
:warning: GitHub issue #35693 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35977: GH-35693: [MATLAB] Add Valid as a name-value pair on the arrow.array.Array superclass constructor
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35977:
URL: https://github.com/apache/arrow/pull/35977#issuecomment-1581259118
:warning: GitHub issue #35693 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35977:
URL: https://github.com/apache/arrow/pull/35977#issuecomment-1581399633
:warning: GitHub issue #35693 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35977:
URL: https://github.com/apache/arrow/pull/35977#issuecomment-1581411745
:warning: GitHub issue #35693 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sgilmore10 commented on a diff in pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #35977:
URL: https://github.com/apache/arrow/pull/35977#discussion_r1224229045
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=false);
+ expectedValidElements = [true; true; true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function LogicalValid(testCase)
+ data = [1 2 3];
+
+ % Supply a scalar true value for Valid
+ validElements = parseValidElements(data, Valid=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a scalar false value for Valid
+ validElements = parseValidElements(data, Valid=false);
+ expectedValidElements = [false; false; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a logical vector for Valid
+ validElements = parseValidElements(data, Valid=[false; true; true]);
+ expectedValidElements = [false; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function NumericValid(testCase)
+ data = [1 2 3];
+
+ % Supply 1 valid index for Valid
+ validElements = parseValidElements(data, Valid=2);
+ expectedValidElements = [false; true; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a numeric vector for Valid
+ validElements = parseValidElements(data, Valid=[1 3]);
+ expectedValidElements = [true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function ComplexValidIndicesError(testCase)
+ % Verify parseValidElements errors if Valid is a numeric array
+ % containing complex numbers.
Review Comment:
I think it's ok if we leave the comments indented. Both styles are acceptable in MATLAB. I talked with @kevingurney about this yesterday and he prefers indenting these comments. I don't have a strong opinion, so I'm happy to go with indenting them. Does that work for you @kou?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sgilmore10 commented on a diff in pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #35977:
URL: https://github.com/apache/arrow/pull/35977#discussion_r1223053649
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=false);
+ expectedValidElements = [true; true; true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function LogicalValid(testCase)
+ data = [1 2 3];
+
+ % Supply a scalar true value for Valid
+ validElements = parseValidElements(data, Valid=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a scalar false value for Valid
+ validElements = parseValidElements(data, Valid=false);
+ expectedValidElements = [false; false; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a logical vector for Valid
+ validElements = parseValidElements(data, Valid=[false; true; true]);
+ expectedValidElements = [false; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function NumericValid(testCase)
+ data = [1 2 3];
+
+ % Supply 1 valid index for Valid
+ validElements = parseValidElements(data, Valid=2);
+ expectedValidElements = [false; true; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a numeric vector for Valid
+ validElements = parseValidElements(data, Valid=[1 3]);
+ expectedValidElements = [true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function ComplexValidIndicesError(testCase)
+ % Verify parseValidElements errors if Valid is a numeric array
+ % containing complex numbers.
Review Comment:
Either comment style works well. Typically, comments that describe the overall behavior of a function are not indented. These comments become the help text that appears when you run:
```MATLAB
>> help <functionName>
```
Here's a [link](https://www.mathworks.com/help/matlab/matlab_prog/add-help-for-your-program.html) to a documentation page about help text if you are interested in reading more about it. However, it's fine for these comments to be indented, so it's really just a personal preference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35977:
URL: https://github.com/apache/arrow/pull/35977#issuecomment-1586041925
Benchmark runs are scheduled for baseline = 6edfcc591bac3f6f29117ab4fa1161aafc07a9a7 and contender = 788760570d8182e1eb05a47e8871360515f431be. 788760570d8182e1eb05a47e8871360515f431be is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/9d8ccdc35d7d4124b967eaf457732923...0cee706745254baf8675f0ef51c92e34/)
[Finished :arrow_down:0.59% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/95a10164e9184e6ab8010a5c9c1c2eae...1626c471d20745cf8532d232b883603d/)
[Finished :arrow_down:0.33% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/50dbe9e086994053b9e64b0cec45c722...c42d83ded29d481496262262a3159081/)
[Finished :arrow_down:0.27% :arrow_up:0.42%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/79d6e9ae008b4496a9c8d0c35e176843...d35b6b1646354ae8901286fa0cd8645b/)
Buildkite builds:
[Finished] [`78876057` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/3019)
[Finished] [`78876057` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3055)
[Finished] [`78876057` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/3020)
[Finished] [`78876057` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/3045)
[Finished] [`6edfcc59` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/3018)
[Finished] [`6edfcc59` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3054)
[Finished] [`6edfcc59` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/3019)
[Finished] [`6edfcc59` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/3044)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou merged pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #35977:
URL: https://github.com/apache/arrow/pull/35977
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #35977:
URL: https://github.com/apache/arrow/pull/35977#discussion_r1223553259
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=false);
+ expectedValidElements = [true; true; true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function LogicalValid(testCase)
+ data = [1 2 3];
+
+ % Supply a scalar true value for Valid
+ validElements = parseValidElements(data, Valid=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a scalar false value for Valid
+ validElements = parseValidElements(data, Valid=false);
+ expectedValidElements = [false; false; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a logical vector for Valid
+ validElements = parseValidElements(data, Valid=[false; true; true]);
+ expectedValidElements = [false; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function NumericValid(testCase)
+ data = [1 2 3];
+
+ % Supply 1 valid index for Valid
+ validElements = parseValidElements(data, Valid=2);
+ expectedValidElements = [false; true; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a numeric vector for Valid
+ validElements = parseValidElements(data, Valid=[1 3]);
+ expectedValidElements = [true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function ComplexValidIndicesError(testCase)
+ % Verify parseValidElements errors if Valid is a numeric array
+ % containing complex numbers.
Review Comment:
> Typically, comments that describe the overall behavior of a function are not indented.
Oh, I see. Then could you revert my suggestions? Sorry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sgilmore10 commented on a diff in pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #35977:
URL: https://github.com/apache/arrow/pull/35977#discussion_r1225393688
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=false);
+ expectedValidElements = [true; true; true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function LogicalValid(testCase)
+ data = [1 2 3];
+
+ % Supply a scalar true value for Valid
+ validElements = parseValidElements(data, Valid=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a scalar false value for Valid
+ validElements = parseValidElements(data, Valid=false);
+ expectedValidElements = [false; false; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a logical vector for Valid
+ validElements = parseValidElements(data, Valid=[false; true; true]);
+ expectedValidElements = [false; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function NumericValid(testCase)
+ data = [1 2 3];
+
+ % Supply 1 valid index for Valid
+ validElements = parseValidElements(data, Valid=2);
+ expectedValidElements = [false; true; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a numeric vector for Valid
+ validElements = parseValidElements(data, Valid=[1 3]);
+ expectedValidElements = [true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function ComplexValidIndicesError(testCase)
+ % Verify parseValidElements errors if Valid is a numeric array
+ % containing complex numbers.
Review Comment:
Thanks Kou for working with us!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sgilmore10 commented on a diff in pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #35977:
URL: https://github.com/apache/arrow/pull/35977#discussion_r1224685967
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=false);
+ expectedValidElements = [true; true; true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function LogicalValid(testCase)
+ data = [1 2 3];
+
+ % Supply a scalar true value for Valid
+ validElements = parseValidElements(data, Valid=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a scalar false value for Valid
+ validElements = parseValidElements(data, Valid=false);
+ expectedValidElements = [false; false; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a logical vector for Valid
+ validElements = parseValidElements(data, Valid=[false; true; true]);
+ expectedValidElements = [false; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function NumericValid(testCase)
+ data = [1 2 3];
+
+ % Supply 1 valid index for Valid
+ validElements = parseValidElements(data, Valid=2);
+ expectedValidElements = [false; true; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a numeric vector for Valid
+ validElements = parseValidElements(data, Valid=[1 3]);
+ expectedValidElements = [true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function ComplexValidIndicesError(testCase)
+ % Verify parseValidElements errors if Valid is a numeric array
+ % containing complex numbers.
Review Comment:
Hi @kou,
We discussed this a bit further and in retrospect, we think it's probably best to stick with MATLAB's auto-indentation behavior (which is to not indent top-level function comments). This should help reduce confusion for future contributors if they stick with the MATLAB default indentation rules.
Sorry it took us some time to figure out the right path forward, but thank you for bringing this up. I'll make the changes in this PR.
Best,
Sarah
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #35977:
URL: https://github.com/apache/arrow/pull/35977#discussion_r1224795319
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=false);
+ expectedValidElements = [true; true; true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function LogicalValid(testCase)
+ data = [1 2 3];
+
+ % Supply a scalar true value for Valid
+ validElements = parseValidElements(data, Valid=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a scalar false value for Valid
+ validElements = parseValidElements(data, Valid=false);
+ expectedValidElements = [false; false; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a logical vector for Valid
+ validElements = parseValidElements(data, Valid=[false; true; true]);
+ expectedValidElements = [false; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function NumericValid(testCase)
+ data = [1 2 3];
+
+ % Supply 1 valid index for Valid
+ validElements = parseValidElements(data, Valid=2);
+ expectedValidElements = [false; true; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a numeric vector for Valid
+ validElements = parseValidElements(data, Valid=[1 3]);
+ expectedValidElements = [true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function ComplexValidIndicesError(testCase)
+ % Verify parseValidElements errors if Valid is a numeric array
+ % containing complex numbers.
Review Comment:
OK!
I'll merge this after you push the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #35977: GH-35693: [MATLAB] Add `Valid` as a name-value pair on the `arrow.array.Float64Array` constructor
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #35977:
URL: https://github.com/apache/arrow/pull/35977#discussion_r1222513086
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
Review Comment:
```suggestion
% Verify values for which ismissing returns true for are not
% considered valid.
```
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
Review Comment:
```suggestion
% Verify all elements are treated as Valid wen InferNulls=false is
% provided - including values for which that ismissing returns true.
```
##########
matlab/test/arrow/args/tParseValidElements.m:
##########
@@ -0,0 +1,163 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tParseValidElements < matlab.unittest.TestCase
+ % Tests for arrow.args.parseValidElements
+
+ methods(Test)
+ % Test methods
+ function InferNullsTrue(testCase)
+ % Verify values for which ismissing returns true for are not
+ % considered valid.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; false; true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ data = [1 2 3];
+ validElements = parseValidElements(data, InferNulls=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify all elements are treated as Valid wen InferNulls=false is
+ % provided - including values for which that ismissing returns true.
+ data = [1 NaN 3 NaN 5];
+ validElements = parseValidElements(data, InferNulls=false);
+ expectedValidElements = [true; true; true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function LogicalValid(testCase)
+ data = [1 2 3];
+
+ % Supply a scalar true value for Valid
+ validElements = parseValidElements(data, Valid=true);
+ expectedValidElements = [true; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a scalar false value for Valid
+ validElements = parseValidElements(data, Valid=false);
+ expectedValidElements = [false; false; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a logical vector for Valid
+ validElements = parseValidElements(data, Valid=[false; true; true]);
+ expectedValidElements = [false; true; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function NumericValid(testCase)
+ data = [1 2 3];
+
+ % Supply 1 valid index for Valid
+ validElements = parseValidElements(data, Valid=2);
+ expectedValidElements = [false; true; false];
+ testCase.verifyEqual(validElements, expectedValidElements);
+
+ % Supply a numeric vector for Valid
+ validElements = parseValidElements(data, Valid=[1 3]);
+ expectedValidElements = [true; false; true];
+ testCase.verifyEqual(validElements, expectedValidElements);
+ end
+
+ function ComplexValidIndicesError(testCase)
+ % Verify parseValidElements errors if Valid is a numeric array
+ % containing complex numbers.
Review Comment:
```suggestion
% Verify parseValidElements errors if Valid is a numeric array
% containing complex numbers.
```
Is this natural comment indent style in MATLAB...?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org