feat: add iter/cartesian-power
#1350
Conversation
|
@TheNourhan Would you mind checking the box in the OP indicating that you've read the project contributing guidelines? Thanks! |
There was a problem hiding this comment.
Thanks for working on this @TheNourhan. Looks like this feature addition is missing tests and benchmarks. We won't be able to review until those are added. Thanks!
|
Also, please ensure that you have run |
kgryte
added
Utilities
|
Can you tell me the errors in any file, please? |
| */ | ||
|
|
||
| function iterCartesianPower(x, n) { | ||
| var iter; |
There was a problem hiding this comment.
This should be tab indentation. Make sure all JS files use tab indentation.
| var FLG = false; | ||
| var i = 0; | ||
| var j = []; | ||
| var idx = []; |
There was a problem hiding this comment.
This should have triggered lint errors. Please make sure you have run make init before attempting to commit code.
| var idx = []; | ||
|
|
||
| // Validate input arguments | ||
| if (!Array.isArray(x) && !isTypedArray(x) && !isIterable(x)) { |
There was a problem hiding this comment.
Please have a look through the codebase and match our spacing conventions. Additionally, you should not be checking for iterables, as you are not handling iterables in the implementation. Lastly, check for a collection, rather than using isArray and isTypedArray.
|
|
||
| // Validate input arguments | ||
| if (!Array.isArray(x) && !isTypedArray(x) && !isIterable(x)) { | ||
| throw new TypeError('invalid argument. First argument must be an array-like object.'); |
There was a problem hiding this comment.
Include the value in the error message, similar to L65.
| if (!Array.isArray(x) && !isTypedArray(x) && !isIterable(x)) { | ||
| throw new TypeError('invalid argument. First argument must be an array-like object.'); | ||
| } | ||
| if (!isNumber(n) || isnan(n)) { |
There was a problem hiding this comment.
This check is not necessary given L67.
|
This PR is still missing tests and benchmarks. |
|
I used npm init, the Editorkonfig format and added Testes & Benchmarks, and updated the files you mentioned. Thanks for your feedback. |
|
|
||
| # iterCartesianPower | ||
|
|
||
| > Create an iterator which generates the Cartesian power of an input array-like object. |
There was a problem hiding this comment.
| > Create an iterator which generates the Cartesian power of an input array-like object. | |
| > Create an iterator which returns the Cartesian power. |
|
|
||
| #### iterCartesianPower( x, n ) | ||
|
|
||
| Returns an iterator which generates the Cartesian power of an input array-like object. |
There was a problem hiding this comment.
| Returns an iterator which generates the Cartesian power of an input array-like object. | |
| Returns an iterator which returns the Cartesian power. |
| var n = 2; | ||
| var iterator = iterCartesianPower( x, n ); | ||
|
|
||
| for (const combination of iterator) { |
There was a problem hiding this comment.
Everything should be written in ES5. See also https://github.com/stdlib-js/stdlib/blob/develop/FAQ.md#es2015-and-beyond.
There was a problem hiding this comment.
You can perform manual iteration. See other iterator packages for examples.
| var x = ['a', 'b', 'c']; | ||
| var n = 2; | ||
| var iterator = iterCartesianPower( x, n ); |
There was a problem hiding this comment.
| var x = ['a', 'b', 'c']; | |
| var n = 2; | |
| var iterator = iterCartesianPower( x, n ); | |
| var x = [ 'a', 'b', 'c' ]; | |
| var iterator = iterCartesianPower( x, 2 ); |
| ## Notes | ||
|
|
||
| - The function expects the first argument x to be an array-like object. | ||
| - The second argument n must be a non-negative integer. | ||
| - The returned iterator will generate all possible combinations of n elements from the input array x. | ||
|
|
There was a problem hiding this comment.
| ## Notes | |
| - The function expects the first argument x to be an array-like object. | |
| - The second argument n must be a non-negative integer. | |
| - The returned iterator will generate all possible combinations of n elements from the input array x. | |
We don't typically explain edge cases. For the last note, this is implied by the concept of the Cartesian power.
| <section class="examples"> | ||
|
|
||
| ## Examples | ||
|
|
There was a problem hiding this comment.
Do not include spurious newlines unless specified to do so by linting.
|
|
||
| ``` | ||
|
|
||
| ```javascript |
There was a problem hiding this comment.
Don't split up examples like this. This does not match the conventions in this project.
| var n = 2; | ||
| var iterator = iterCartesianPower(x, n); | ||
|
|
||
| for (const combination of iterator) { |
| <section class="related"> | ||
|
|
||
| * * * | ||
|
|
||
| ## See Also | ||
|
|
||
| - <span class="package-name">[`@stdlib/array/from-iterator`][@stdlib/array/from-iterator]</span><span class="delimiter">: </span><span class="description">create (or fill) an array from an iterator.</span> | ||
| - <span class="package-name">[`@stdlib/iter/datespace`][@stdlib/iter/datespace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns evenly spaced dates over a specified interval.</span> | ||
| - <span class="package-name">[`@stdlib/iter/incrspace`][@stdlib/iter/incrspace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns evenly spaced numbers according to a specified increment.</span> | ||
| - <span class="package-name">[`@stdlib/iter/logspace`][@stdlib/iter/logspace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns evenly spaced numbers on a log scale.</span> | ||
| - <span class="package-name">[`@stdlib/iter/step`][@stdlib/iter/step]</span><span class="delimiter">: </span><span class="description">create an iterator which returns a sequence of numbers according to a specified increment.</span> | ||
| - <span class="package-name">[`@stdlib/iter/unitspace`][@stdlib/iter/unitspace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns numbers incremented by one.</span> |
There was a problem hiding this comment.
| <section class="related"> | |
| * * * | |
| ## See Also | |
| - <span class="package-name">[`@stdlib/array/from-iterator`][@stdlib/array/from-iterator]</span><span class="delimiter">: </span><span class="description">create (or fill) an array from an iterator.</span> | |
| - <span class="package-name">[`@stdlib/iter/datespace`][@stdlib/iter/datespace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns evenly spaced dates over a specified interval.</span> | |
| - <span class="package-name">[`@stdlib/iter/incrspace`][@stdlib/iter/incrspace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns evenly spaced numbers according to a specified increment.</span> | |
| - <span class="package-name">[`@stdlib/iter/logspace`][@stdlib/iter/logspace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns evenly spaced numbers on a log scale.</span> | |
| - <span class="package-name">[`@stdlib/iter/step`][@stdlib/iter/step]</span><span class="delimiter">: </span><span class="description">create an iterator which returns a sequence of numbers according to a specified increment.</span> | |
| - <span class="package-name">[`@stdlib/iter/unitspace`][@stdlib/iter/unitspace]</span><span class="delimiter">: </span><span class="description">create an iterator which returns numbers incremented by one.</span> | |
| <section class="related"> |
| <!-- <related-links> --> | ||
|
|
||
| [@stdlib/array/from-iterator]: https://www.npmjs.com/package/@stdlib/array-from-iterator | ||
|
|
||
| [@stdlib/iter/datespace]: https://github.com/stdlib-js/iter/tree/main/datespace | ||
|
|
||
| [@stdlib/iter/incrspace]: https://github.com/stdlib-js/iter/tree/main/incrspace | ||
|
|
||
| [@stdlib/iter/logspace]: https://github.com/stdlib-js/iter/tree/main/logspace | ||
|
|
||
| [@stdlib/iter/step]: https://github.com/stdlib-js/iter/tree/main/step | ||
|
|
||
| [@stdlib/iter/unitspace]: https://github.com/stdlib-js/iter/tree/main/unitspace | ||
|
|
||
| <!-- </related-links> --> | ||
|
|
There was a problem hiding this comment.
| <!-- <related-links> --> | |
| [@stdlib/array/from-iterator]: https://www.npmjs.com/package/@stdlib/array-from-iterator | |
| [@stdlib/iter/datespace]: https://github.com/stdlib-js/iter/tree/main/datespace | |
| [@stdlib/iter/incrspace]: https://github.com/stdlib-js/iter/tree/main/incrspace | |
| [@stdlib/iter/logspace]: https://github.com/stdlib-js/iter/tree/main/logspace | |
| [@stdlib/iter/step]: https://github.com/stdlib-js/iter/tree/main/step | |
| [@stdlib/iter/unitspace]: https://github.com/stdlib-js/iter/tree/main/unitspace | |
| <!-- </related-links> --> |
|
|
||
| // MODULES // | ||
|
|
||
| const bench = require( '@stdlib/bench' ); |
There was a problem hiding this comment.
Please run linting over this file. See https://github.com/stdlib-js/stdlib/tree/develop/tools/make/lib/lint#lint-javascript-benchmarks.
| v = iterator.next(); | ||
| if ( v.done ) { | ||
| b.fail( 'should iterate over all elements' ); | ||
| } | ||
| } | ||
| b.toc(); | ||
| if ( v.done ) { | ||
| b.pass( 'benchmark finished' ); | ||
| } else { | ||
| b.fail( 'should have finished iteration' ); |
There was a problem hiding this comment.
This logic is not correct. You actually need to ensure that v.done does not return true. The point is to test per iteration performance, not how long it can generate all combinations. If folks want to generate all combos, they should just use @stdlib/array/cartesian-power. Instead, we want to know how long it takes a single iteration.
|
|
||
| Parameters | ||
| ---------- | ||
| x : ArrayLike<T> | Iterable<T> |
There was a problem hiding this comment.
This is not TypeScript. Please read https://github.com/stdlib-js/stdlib/blob/develop/docs/repl_text.md
| res.push( x[ idx[ k ] ] ); | ||
| } | ||
| // Update indices | ||
| for ( var k = n - 1; k >= 0; k-- ) { |
There was a problem hiding this comment.
We explicitly hoist declared variables. See other packages for examples.
| "linspace", | ||
| "linear", | ||
| "sequence", | ||
| "monotonic", | ||
| "arange", | ||
| "range", |
There was a problem hiding this comment.
| "linspace", | |
| "linear", | |
| "sequence", | |
| "monotonic", | |
| "arange", | |
| "range", | |
| "cartesian", | |
| "combination", | |
| "permutation", |
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| 'use strict'; |
There was a problem hiding this comment.
Please match the spacing and style conventions of other packages in the project.
| tape( 'the function returns an iterator which generates all possible combinations of length `n` containing elements from the input array', function test( t ) { | ||
| var expected = [ [ 'a', 'a' ], [ 'a', 'b' ], [ 'a', 'c' ], [ 'b', 'a' ], [ 'b', 'b' ], [ 'b', 'c' ], [ 'c', 'a' ], [ 'c', 'b' ], [ 'c', 'c' ] ]; | ||
| var actual = []; | ||
| for ( var v of iterCartesianPower( [ 'a', 'b', 'c' ], 2) ) { |
|
@TheNourhan In general, I suggest trying to make your contribution look as closely to other packages as possible. We author everything in ES5 and we enforce various lint conventions, which are not currently adhered to. |
|
I made all the changes you mentioned except for the benchmark file and lint errors I will work on them after I fix the installation errors in my project because the make command does not work well in my env. |
|
I've made all the changes you mentioned |
|
@TheNourhan Great! Thanks for the update. We'll try and review within the next day or so. |
|
I'm going to close this PR and open a new one, I reinstalled the project and fixed all the errors |
|
@TheNourhan Sounds good. Feel free to do so. |
|
@TheNourhan Is this PR obsolete? Based on your prior comments, you were planning on submitting a new PR. |
Resolves #1336
Description
The purpose of this pull request is to implement the iterCartesianPower function, which generates the Cartesian power of an input array-like object.
This pull request:
libfolder.docsfolder, including:docs/repl.txtfile.docs/typesfolder.Related Issues
This pull request:
@stdlib/iter/cartesian-power#1336Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers