Skip to content

Conversation

@ryankert01
Copy link
Contributor

@ryankert01 ryankert01 commented Dec 17, 2025

Purpose of PR

default has been changed to f32, so the bits result would be 32*2=64
solution: Make the test not init the executor to default, instead init to a predetermined f64

Related Issues or PRs

Closes #733

Changes Made

  • Bug fix
  • New feature
  • Refactoring
  • Documentation
  • Test
  • CI/CD pipeline
  • Other

Breaking Changes

  • Yes
  • No

Checklist

  • Added or updated unit tests for all changes
  • Added or updated documentation for all changes
  • Successfully built and ran all unit tests or manual tests locally
  • PR title follows "MAHOUT-XXX: Brief Description" format (if related to an issue)
  • Code follows ASF guidelines

TODO

  • add a test for original new() fp32, bring back the coverage

Signed-off-by: Hsien-Cheng Huang <[email protected]>
@ryankert01 ryankert01 changed the base branch from main to dev-qdp December 17, 2025 04:19
Copy link
Contributor Author

@ryankert01 ryankert01 left a comment

Choose a reason for hiding this comment

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

sorry for my vscode rust extension is changing a lot of our codespace, I've highlighted the changes.
PTAL @guan404ming @rich7420 @400Ping @machichima

assert_eq!(
tensor.dtype.bits, 128,
"Should be 128 bits (2x64-bit floats)"
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the failed test

Choose a reason for hiding this comment

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

I am thinking as we change the precision to float32 by default, should we set the expected value to 2x32 rather than change the precision back to float64?

Copy link

@400Ping 400Ping Dec 18, 2025

Choose a reason for hiding this comment

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

see this, working on float32 right now.

println!("Testing DLPack tensor metadata...");

let engine = match QdpEngine::new(0) {
let engine = match QdpEngine::new_with_precision(0, Precision::Float64) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I changed

@ryankert01 ryankert01 changed the title Fix test bug1 [QDP] fix test bug Dec 17, 2025
@400Ping
Copy link

400Ping commented Dec 17, 2025

Can you provide the reproduction scrip you run for the error?

@ryankert01
Copy link
Contributor Author

ryankert01 commented Dec 17, 2025

Can you provide the reproduction scrip you run for the error?

cargo test

@400Ping
Copy link

400Ping commented Dec 17, 2025

strange, my local test passed.

@ryankert01
Copy link
Contributor Author

@400Ping As I'm concerned, there's a PR change default from fp64 to fp32
Correct me if I'm wrong

@machichima
Copy link

I can reproduce the error on the newest dev-qdp branch

image

@400Ping
Copy link

400Ping commented Dec 18, 2025

@400Ping As I'm concerned, there's a PR change default from fp64 to fp32 Correct me if I'm wrong

Yes, I am also working on the follow up as well.

@ryankert01
Copy link
Contributor Author

cc @guan404ming @rich7420

@rich7420
Copy link
Contributor

local test all passed.

Copy link

@machichima machichima left a comment

Choose a reason for hiding this comment

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

LGTM

@rich7420 rich7420 merged commit 828d379 into apache:dev-qdp Dec 20, 2025
2 checks passed
@ryankert01 ryankert01 deleted the fix-test-bug1 branch December 21, 2025 01:59
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.

[QDP] [Bug] fix test bug

4 participants