Skip to content

Conversation

Krmjn09
Copy link
Collaborator

@Krmjn09 Krmjn09 commented Jul 25, 2025

Created 4 fields with these entry values

for (int i = 0; i < 10; ++i) {
*intField = i;
*floatField = static_cast(i * i);
*doubleField = i * 0.5;
*stringField = "entry_" + std::to_string(i);
writer->Fill();
}

and then checked the rntuple_test.root file in rntuple_test.js
Its working successfully fine as you can see in the below result
Screenshot 2025-07-25 125927

@Krmjn09 Krmjn09 requested review from linev and silverweed July 25, 2025 07:54
try {
const value = readEntry(rntuple, field, entryIndex),
expectedValue = expected[entryIndex][field];
if (value !== expectedValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pay attention when comparing floating point numbers with ===: due to floating point errors you might get some false negatives where two numbers that should be the same are not identical. So usually instead of x === y you want to use Math.abs(x - y) < EPSILON, where EPSILON is a small number (like 1e-10).
And I believe this is also true for "integers" in javascript, as all numbers are floating points unless you specifically use special types.

9: { IntField: 9, FloatField: 81, DoubleField: 4.5, StringField: 'entry_9' }
};

for (const entryIndex of [0, 9]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you have few entries and you constructed them in a way that's easy to programmatically reproduce, you can also loop over all the entries and calculate the expected values in the loop, rather than hardcoding it in expected

@Krmjn09 Krmjn09 requested a review from silverweed July 27, 2025 02:17
Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.
Please fix the eslint warning before we merge

@Krmjn09 Krmjn09 requested a review from silverweed July 28, 2025 09:44
@silverweed silverweed merged commit ba440dc into root-project:dev Jul 28, 2025
24 checks passed
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.

2 participants