feat: add array/complex-factory#11272
feat: add array/complex-factory#11272udaykakade25 wants to merge 8 commits intostdlib-js:developfrom
array/complex-factory#11272Conversation
udaykakade25
changed the title
array/complex-factoryarray/complex-factory
|
/stdlib update-copyright-years |
stdlib-bot
added
the
bot: In Progress
stdlib-bot
removed
the
bot: In Progress
Signed-off-by: Uday Kakade <141299403+udaykakade25@users.noreply.github.com>
Signed-off-by: Uday Kakade <141299403+udaykakade25@users.noreply.github.com>
There was a problem hiding this comment.
This file should not be modified in this PR. It is okay that there are spellcheck lint warnings. If we want to update this, it should be in a separate PR. TL;DR: PRs should, more or less, follow our Git commit message conventions, which means following conventional commits. Which means that PRs should be single-focused. By making updates to lint configuration, you are mixing concerns.
| fromArray = makeFromArray( getReal, getImag ); | ||
|
|
||
|
|
||
| // FUNCTIONS // |
There was a problem hiding this comment.
Section headers do not go in function bodies.
| * @throws {RangeError} must provide valid arguments | ||
| * @returns {Function} complex number array constructor | ||
| */ | ||
| function factory( dtype, FloatArray, ComplexScalar, getReal, getImag, reinterpret ) { |
There was a problem hiding this comment.
Why are we forcing users to provide anything other than the dtype? Couldn't we have all the above in a look-up table internally? What is the design philosophy/rationale here?
| typeof value._length === 'number' && // eslint-disable-line no-underscore-dangle | ||
| typeof value._buffer === 'object' // eslint-disable-line no-underscore-dangle |
There was a problem hiding this comment.
This alone is too weak a check. There is a reason why we check the constructor name in, e.g., array/complex128.
| typeof value === 'function' && | ||
| typeof value.BYTES_PER_ELEMENT === 'number' && | ||
| value.BYTES_PER_ELEMENT > 0 |
There was a problem hiding this comment.
This is also too weak a check. Why are we not comparing against the constructor name?
| if ( !isComplexArray( this ) ) { | ||
| throw new TypeError( 'invalid invocation. `this` is not a complex number array.' ); | ||
| } | ||
| // eslint-disable-next-line no-warning-comments |
There was a problem hiding this comment.
Don't disable this lint rule. See these lint warnings during linting is desirable.
| tmp.push( getComplex( buf, i ) ); | ||
| } | ||
| tmp.sort( compareFcn ); | ||
| return new ComplexArray( tmp ); |
There was a problem hiding this comment.
Why are we still using new ComplexArray here rather than this.constructor? Currently, you thwart subclassing.
| // MAIN // | ||
|
|
||
| /** | ||
| * Returns a complex number array constructor. |
There was a problem hiding this comment.
The following is a general comment that is more intended for discussion rather than providing something actionable...
The factory approach does work and it will reduce bundle sizes as the implementation would now be centralized.
That said, the factory approach does suffer from one drawback, which is it doesn't allow for inheritance from a parent constructor (e.g., ComplexArray -> Complex128Array). This lack of inheritance means that brand checks are more expensive (i.e., for many operations, we don't care which type of complex typed array we were given; we just want a complex typed array). If we had a parent class, we could simply check x instanceof ComplexArray (i.e., is this value an instance of the parent class?). With the factory approach, we cannot do that, as each ComplexXXArray is a separate class with no common inheritance.
Furthermore, separate classes are treated as objects having different hidden shapes. This then results in JS optimizing compilers treating input arrays as distinct and thus leading the compiler to mark a function as polymorphic. This can degrade performance. This is also true of built-in arrays, so we are not an outlier here, but the factory approach does introduce a potential performance cliff.
I am not sure that a parent class approach is actually feasible, but I at least wanted to document that there are trade-offs associated with a factory approach.
Signed-off-by: Uday Kakade <141299403+udaykakade25@users.noreply.github.com>
3 participants
Resolves none.
Description
This pull request:
array/complex-factorypackage.Related Issues
This pull request has the following related issues:
Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
I used Claude during my R&D by taking references from
array/complex128,array/complex64,array/fix-endian-factory. Later used Claude to automate some of repetitive tasks in docs and benchmark files.@stdlib-js/reviewers