Skip to content

feat: enhance mlx.core.full() TypeScript interface and add comprehensive tests - #548

Closed
sydneyrenee with Copilot wants to merge 5 commits into
mainfrom
copilot/implement-mlx-core-full
Closed

feat: enhance mlx.core.full() TypeScript interface and add comprehensive tests#548
sydneyrenee with Copilot wants to merge 5 commits into
mainfrom
copilot/implement-mlx-core-full

Conversation

Copilot AI commented Oct 16, 2025

Copy link
Copy Markdown
Contributor

✅ Implementation Complete: mlx.core.full() is fully functional

Summary: The mlx.core.full() function was already implemented at the C++ level. This PR enhances the TypeScript interface and adds comprehensive test coverage.

Checklist

  • Review Python implementation (verified in python/src/ops.cpp)
  • Check C++ implementation in node/src/native/array.cc (ALREADY COMPLETE - lines 1486-1566)
  • Verify function is registered in Init() (ALREADY REGISTERED - line 1947)
  • Enhanced TypeScript wrapper to support all value types
  • Added comprehensive test suite (8 test cases)
  • Added JSDoc documentation with examples
  • Verified exports in all index files
  • Confirmed PORT_CHECKLIST.md status (marked as done)
  • Formatted code with prettier
  • Fixed dtype parameter handling to avoid passing undefined to C++

What Was Already Implemented

C++ Native Binding - Full implementation with:

  • Shape, value, dtype, and stream parameters
  • Support for scalar, array, and TypedArray values
  • Broadcasting for array values
  • Intelligent dtype inference
  • Proper error handling

TypeScript Wrapper - Basic wrapper existed
Module Exports - Properly exported throughout
Basic Tests - One test case existed

Enhancements Made

1. Enhanced TypeScript Wrapper (node/src/core/array.ts)

  • Extended signature: number | SupportedTypedArray | MLXArray
  • Made dtype optional with proper typing
  • Added comprehensive JSDoc with dtype inference behavior
  • Proper type routing for different value types
  • Fixed: Now conditionally passes dtype only when explicitly provided (avoids undefined)

2. Comprehensive Test Suite (node/test/core/array.test.ts)

Added 8 new test cases covering:

  • 1D and 2D shapes with scalar values
  • Explicit dtype specification
  • Dtype inference (int32, float32, bool)
  • Array value broadcasting
  • TypedArray value broadcasting

3. Documentation

  • JSDoc comments with description and examples
  • Parameter and return value documentation
  • Enhanced: Documented dtype inference behavior for each value type

4. Code Quality

  • Applied prettier formatting for consistency

API Compatibility

Fully compatible with Python MLX:

// Scalar values
mlx.core.full([2, 3], 2.0)

// Array broadcasting (preserves dtype)
mlx.core.full([3, 2], mlx.core.array([1, 2]))

// Explicit dtype
mlx.core.full([3], 7.5, 'float64')

DType Inference Behavior

The C++ implementation handles dtype inference correctly:

  • MLXArray values: Preserves source array dtype when not specified
  • TypedArray values: Infers from TypedArray type when not specified
  • Scalar values: Infers int32 for integers, float32 for floats

TypeScript now conditionally passes dtype parameter to avoid undefined values.

Verification

All features match Python MLX API:

  • ✅ Scalar values with dtype inference
  • ✅ Array values with broadcasting
  • ✅ TypedArray values
  • ✅ Optional dtype parameter
  • ✅ Stream parameter support

Files Changed

  • node/src/core/array.ts - Enhanced wrapper + JSDoc + dtype handling fix
  • node/test/core/array.test.ts - Added 8 comprehensive tests
  • node/package.json - Updated dev dependencies
  • node/package-lock.json - Dependency updates
Original prompt

This section details on the original issue you should resolve

<issue_title>Implement mlx.core.full()</issue_title>
<issue_description>## 🎯 Implement mlx.core.full()

Priority: medium | Module: core | Type: other | Category: core


📋 Quick Reference

Item Value
Python Source python/src/ops.cpp
Node Target node/src/native/array.cc
Test File node/test/ops.test.js
C++ Namespace mlx::core

🚀 Step-by-Step Implementation

Step 1: Review Python Implementation

# See the Python binding
grep -B 5 -A 30 '"full"' python/src/ops.cpp

Step 2: Implement in Node.js

File to edit: node/src/native/array.cc

Napi::Value Full(const Napi::CallbackInfo& info) {
  auto env = info.Env();
  auto* addon = static_cast<mlx::node::AddonData*>(info.Data());
  
  try {
    mlx::node::Runtime::Instance().EnsureMetalInit();
  } catch (const std::exception& e) {
    Napi::Error::New(env, e.what()).ThrowAsJavaScriptException();
    return env.Null();
  }
  
  // TODO: Parse arguments based on Python signature
  // Check python/src/python/src/ops.cpp for the exact signature
  
  // Example: Parse array argument
  auto* wrapper = UnwrapArray(env, info[0]);
  if (!wrapper) return env.Null();
  const auto& a = wrapper->tensor();
  
  // Parse stream
  auto stream = mlx::core::default_stream(mlx::core::default_device());
  // (adjust index based on number of args)
  if (info.Length() > 1) {
    stream = mlx::node::ParseStreamOrDevice(env, info[info.Length() - 1], *addon);
    if (env.IsExceptionPending()) return env.Null();
  }
  
  try {
    auto result = mlx::core::full(/* args */, stream);
    return WrapArray(env, std::make_shared<mlx::core::array>(std::move(result)));
  } catch (const std::exception& e) {
    Napi::Error::New(env, std::string("full failed: ") + e.what())
        .ThrowAsJavaScriptException();
    return env.Null();
  }
}

Step 3: Register the Function

Add to the Init() function at bottom of node/src/native/array.cc:

core.Set("full", Napi::Function::New(env, Full, "full", &data));

Step 4: Add Tests

File: node/test/ops.test.js

const mx = require('..');

describe('mlx.core.full', () => {
  it('should work correctly', () => {
    // TODO: Add test based on Python tests
    // const a = mx.core.array([1, 2, 3]);
    // const result = mx.core.full(a);
    // expect(result).toBeDefined();
  });
});

📚 Resources

Similar Implementations

Look at these in node/src/native/array.cc:

  • Unary ops: Exp(), Log(), Sin(), Cos()
  • Binary ops: Add(), Multiply(), Subtract()
  • Reductions: Sum(), Mean(), Max(), Min()

Common Patterns

Parse Array:

auto* wrapper = UnwrapArray(env, info[0]);
if (!wrapper) return env.Null();
const auto& a = wrapper->tensor();

Parse Stream:

auto stream = mlx::core::default_stream(mlx::core::default_device());
if (info.Length() > N) {
  stream = mlx::node::ParseStreamOrDevice(env, info[N], *addon);
}

Return Array:

return WrapArray(env, std::make_shared<mlx::core::array>(std::move(result)));

✅ Completion Checklist

  • Reviewed Python implementation
  • Implemented function in node/src/native/array.cc
  • Registered in Init()
  • Added tests in node/test/ops.test.js
  • Builds: cd node && npm run build
  • Tests pass: npm test
  • Updated docs/API_CHECKLIST.md

Auto-generated for MLX Node.js API completeness
</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #505


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 3 commits October 16, 2025 04:14
Co-authored-by: sydneyrenee <188732394+sydneyrenee@users.noreply.github.com>
…ive tests

Co-authored-by: sydneyrenee <188732394+sydneyrenee@users.noreply.github.com>
Co-authored-by: sydneyrenee <188732394+sydneyrenee@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement mlx.core.full() function feat: enhance mlx.core.full() TypeScript interface and add comprehensive tests Oct 16, 2025
Copilot AI requested a review from sydneyrenee October 16, 2025 04:23
@sydneyrenee
sydneyrenee requested a review from Copilot October 16, 2025 05:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the TypeScript interface for mlx.core.full() to support MLXArray and TypedArray values in addition to scalar numbers, bringing it to parity with the underlying C++ implementation and Python MLX API. The dtype parameter is now optional with automatic inference, and comprehensive test coverage has been added.

Key Changes:

  • Extended full() signature to accept number | SupportedTypedArray | MLXArray values with optional dtype inference
  • Added 8 comprehensive test cases covering scalar values, explicit dtypes, automatic inference, and array broadcasting
  • Applied code formatting improvements across test files

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
node/src/core/array.ts Enhanced full() function signature and implementation to support MLXArray/TypedArray values with optional dtype; added JSDoc documentation
node/test/core/array.test.ts Added comprehensive test suite for full() function and applied prettier formatting to existing tests
node/package.json Added devDependencies for TypeScript type definitions and formatted keywords array
Files not reviewed (1)
  • node/package-lock.json: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread node/src/core/array.ts
@sydneyrenee

Copy link
Copy Markdown
Contributor

@copilot Address this issue: When dtype is undefined and value is an MLXArray or TypedArray, the addon.full function receives undefined as the dtype parameter. Consider documenting whether the C++ implementation handles dtype inference correctly for array values, or add explicit dtype inference logic in TypeScript before calling addon.full to ensure consistent behavior.

Co-authored-by: sydneyrenee <188732394+sydneyrenee@users.noreply.github.com>

Copilot AI commented Oct 16, 2025

Copy link
Copy Markdown
Contributor Author

@copilot Address this issue: When dtype is undefined and value is an MLXArray or TypedArray, the addon.full function receives undefined as the dtype parameter. Consider documenting whether the C++ imp...

Fixed in commit 7b4a8c9. The TypeScript wrapper now conditionally passes the dtype parameter only when explicitly provided, avoiding undefined values. The C++ implementation correctly handles dtype inference for all value types:

  • MLXArray values: preserves source array dtype
  • TypedArray values: infers from TypedArray type
  • Scalar values: infers int32 for integers, float32 for floats

Enhanced JSDoc documentation now clearly describes this behavior.

@sydneyrenee
sydneyrenee marked this pull request as ready for review October 16, 2025 06:02
@sydneyrenee
sydneyrenee requested a review from Copilot October 16, 2025 06:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • node/package-lock.json: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@sydneyrenee

Copy link
Copy Markdown
Contributor

✅ Implementation Complete: mlx.core.full() is fully functional

Summary: The mlx.core.full() function was already implemented at the C++ level. This PR enhances the TypeScript interface and adds comprehensive test coverage.

Checklist

  • Review Python implementation (verified in python/src/ops.cpp)
  • Check C++ implementation in node/src/native/array.cc (ALREADY COMPLETE - lines 1486-1566)
  • Verify function is registered in Init() (ALREADY REGISTERED - line 1947)
  • Enhanced TypeScript wrapper to support all value types
  • Added comprehensive test suite (8 test cases)
  • Added JSDoc documentation with examples
  • Verified exports in all index files
  • Confirmed PORT_CHECKLIST.md status (marked as done)
  • Formatted code with prettier
  • Fixed dtype parameter handling to avoid passing undefined to C++

What Was Already Implemented

C++ Native Binding - Full implementation with:

  • Shape, value, dtype, and stream parameters
  • Support for scalar, array, and TypedArray values
  • Broadcasting for array values
  • Intelligent dtype inference
  • Proper error handling

TypeScript Wrapper - Basic wrapper existed
Module Exports - Properly exported throughout
Basic Tests - One test case existed

Enhancements Made

1. Enhanced TypeScript Wrapper (node/src/core/array.ts)

  • Extended signature: number | SupportedTypedArray | MLXArray
  • Made dtype optional with proper typing
  • Added comprehensive JSDoc with dtype inference behavior
  • Proper type routing for different value types
  • Fixed: Now conditionally passes dtype only when explicitly provided (avoids undefined)

2. Comprehensive Test Suite (node/test/core/array.test.ts)

Added 8 new test cases covering:

  • 1D and 2D shapes with scalar values
  • Explicit dtype specification
  • Dtype inference (int32, float32, bool)
  • Array value broadcasting
  • TypedArray value broadcasting

3. Documentation

  • JSDoc comments with description and examples
  • Parameter and return value documentation
  • Enhanced: Documented dtype inference behavior for each value type

4. Code Quality

  • Applied prettier formatting for consistency

API Compatibility

Fully compatible with Python MLX:

// Scalar values
mlx.core.full([2, 3], 2.0)

// Array broadcasting (preserves dtype)
mlx.core.full([3, 2], mlx.core.array([1, 2]))

// Explicit dtype
mlx.core.full([3], 7.5, 'float64')

DType Inference Behavior

The C++ implementation handles dtype inference correctly:

  • MLXArray values: Preserves source array dtype when not specified
  • TypedArray values: Infers from TypedArray type when not specified
  • Scalar values: Infers int32 for integers, float32 for floats

TypeScript now conditionally passes dtype parameter to avoid undefined values.

Verification

All features match Python MLX API:

  • ✅ Scalar values with dtype inference
  • ✅ Array values with broadcasting
  • ✅ TypedArray values
  • ✅ Optional dtype parameter
  • ✅ Stream parameter support

Files Changed

  • node/src/core/array.ts - Enhanced wrapper + JSDoc + dtype handling fix
  • node/test/core/array.test.ts - Added 8 comprehensive tests
  • node/package.json - Updated dev dependencies
  • node/package-lock.json - Dependency updates
Original prompt

This section details on the original issue you should resolve

<issue_title>Implement mlx.core.full()</issue_title>
<issue_description>## 🎯 Implement mlx.core.full()

Priority: medium | Module: core | Type: other | Category: core


📋 Quick Reference

Item Value
Python Source python/src/ops.cpp
Node Target node/src/native/array.cc
Test File node/test/ops.test.js
C++ Namespace mlx::core

🚀 Step-by-Step Implementation

Step 1: Review Python Implementation

# See the Python binding
grep -B 5 -A 30 '"full"' python/src/ops.cpp

Step 2: Implement in Node.js

File to edit: node/src/native/array.cc

Napi::Value Full(const Napi::CallbackInfo& info) {
  auto env = info.Env();
  auto* addon = static_cast<mlx::node::AddonData*>(info.Data());
  
  try {
    mlx::node::Runtime::Instance().EnsureMetalInit();
  } catch (const std::exception& e) {
    Napi::Error::New(env, e.what()).ThrowAsJavaScriptException();
    return env.Null();
  }
  
  // TODO: Parse arguments based on Python signature
  // Check python/src/python/src/ops.cpp for the exact signature
  
  // Example: Parse array argument
  auto* wrapper = UnwrapArray(env, info[0]);
  if (!wrapper) return env.Null();
  const auto& a = wrapper->tensor();
  
  // Parse stream
  auto stream = mlx::core::default_stream(mlx::core::default_device());
  // (adjust index based on number of args)
  if (info.Length() > 1) {
    stream = mlx::node::ParseStreamOrDevice(env, info[info.Length() - 1], *addon);
    if (env.IsExceptionPending()) return env.Null();
  }
  
  try {
    auto result = mlx::core::full(/* args */, stream);
    return WrapArray(env, std::make_shared<mlx::core::array>(std::move(result)));
  } catch (const std::exception& e) {
    Napi::Error::New(env, std::string("full failed: ") + e.what())
        .ThrowAsJavaScriptException();
    return env.Null();
  }
}

Step 3: Register the Function

Add to the Init() function at bottom of node/src/native/array.cc:

core.Set("full", Napi::Function::New(env, Full, "full", &data));

Step 4: Add Tests

File: node/test/ops.test.js

const mx = require('..');

describe('mlx.core.full', () => {
  it('should work correctly', () => {
    // TODO: Add test based on Python tests
    // const a = mx.core.array([1, 2, 3]);
    // const result = mx.core.full(a);
    // expect(result).toBeDefined();
  });
});

📚 Resources

Similar Implementations

Look at these in node/src/native/array.cc:

  • Unary ops: Exp(), Log(), Sin(), Cos()
  • Binary ops: Add(), Multiply(), Subtract()
  • Reductions: Sum(), Mean(), Max(), Min()

Common Patterns

Parse Array:

auto* wrapper = UnwrapArray(env, info[0]);
if (!wrapper) return env.Null();
const auto& a = wrapper->tensor();

Parse Stream:

auto stream = mlx::core::default_stream(mlx::core::default_device());
if (info.Length() > N) {
  stream = mlx::node::ParseStreamOrDevice(env, info[N], *addon);
}

Return Array:

return WrapArray(env, std::make_shared<mlx::core::array>(std::move(result)));

✅ Completion Checklist

  • Reviewed Python implementation
  • Implemented function in node/src/native/array.cc
  • Registered in Init()
  • Added tests in node/test/ops.test.js
  • Builds: cd node && npm run build
  • Tests pass: npm test
  • Updated docs/API_CHECKLIST.md

Auto-generated for MLX Node.js API completeness
</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #505


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@copilot help fix conflicts

Copilot AI commented Oct 16, 2025

Copy link
Copy Markdown
Contributor Author

✅ Implementation Complete: mlx.core.full() is fully functional

Summary: The mlx.core.full() function was already implemented at the C++ level. This PR enhances the TypeScript interface ...

I checked the repository and there are currently no merge conflicts. The working tree is clean and all changes have been committed successfully.

If you're seeing conflicts on the GitHub PR page, please let me know:

  1. Which files have conflicts?
  2. What is the target branch for merging?

I'll be happy to help resolve any conflicts once I know more details.

@sydneyrenee

Copy link
Copy Markdown
Contributor

Closing in favor of manual integration on main: reworked TypeScript wrapper/tests for mlx.core.full() without the noise.

@sydneyrenee
sydneyrenee deleted the copilot/implement-mlx-core-full branch October 16, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement mlx.core.full()

3 participants