-
Notifications
You must be signed in to change notification settings - Fork 80
$file.readBuffer to read the data from disk #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
$file.readBuffer to read the data from disk #343
Conversation
Very good, these numbers look correct! The next step after this would be to get the column type and page uncompressed size from the schema you deserialized in the header. In this case you should find that the column type is 7 (kReal64). |
demo/node/rntuple_test.js
Outdated
|
||
let file = await openFile('https://jsroot.gsi.de/files/tmp/ntpl001_staff.root'); | ||
let file = await openFile('./simple.root'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you using local simple.root
file - please add it to PR, otherwise tests will fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, why did they not fail in the CI? Are the rntuple tests not run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in rntuple_test file are kind of like a checker for the fields and columns if they did not match for ntupl_staff001 it will answer like expected the first field as category but got doubles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the tests are running in my terminal but gives failure instead of Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's passing all checks even if the simple.root is not in GitHub because I haven't still added rntuple_test.js in jsroot ci.yml file yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do that as soon as possible
modules/rntuple.mjs
Outdated
@@ -546,6 +557,30 @@ async function readHeaderFooter(tuple) { | |||
|
|||
tuple.builder.deserializePageList(unzipped_blob); | |||
return true; | |||
|
|||
// Read the first page data | |||
// const firstPage = tuple.builder?.pageLocations?.[0]?.[0]?.pages?.[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you comment out all the new code and changed back the rntuple file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like i don't know how can I add the root file in GitHub once I get to know that I will uncomment the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just git add
the root file in the demo
directory (is that ok @linev)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that one file forcefully correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!! Thankyou for clarification
And yes both my root and test file are present in demo/node directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that even though the test file is in a directory it's not necessarily true that the tests are run from that directory; you should be able to find out the working directory by printing out __dirname
from the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some tests one do:
cd demo/node; node make_image.js
See examples in the jsroot-ci.yml
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used absolute path with __dirname , the issue is resoved now !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote before - much easier to run tests from current directory.
Then simple file path can be used.
console.log(`Field: ${field?.name ?? 'undefined'} | Type: ${field?.typeName ?? 'unknown'}`); | ||
throw new Error('No column descriptor found'); | ||
|
||
const field = tuple.builder.fieldDescriptors?.[firstColumn.fieldId]; | ||
|
||
// Returns the size in bytes of one value based on its type | ||
function getElementSize(typeName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in the future you should get the element size from the Column Descriptor (from the "bits on storage" field)
Thanks @Krmjn09 ! |
first 10 double values from data page