Skip to content

Feature/removed hardcoded column handling #352

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

Merged

Conversation

Krmjn09
Copy link
Collaborator

@Krmjn09 Krmjn09 commented Jul 17, 2025

No description provided.

@Krmjn09 Krmjn09 changed the base branch from master to dev July 17, 2025 10:26
@Krmjn09 Krmjn09 requested review from silverweed and linev July 17, 2025 10:26
for (let i = 0; i < numEntries; ++i) {
const obj = {};
for (const [fieldName, values] of Object.entries(rntuple._clusterData))
obj[fieldName] = values[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to use an intermediate obj rather than directly selector.tgtobj?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yess i could have directly assign the value to selector.tgtobj

I will change this as soon as possible!
Thankyou for pointing out!
Please review @silverweed

@Krmjn09 Krmjn09 requested a review from silverweed July 17, 2025 11:07
@silverweed
Copy link
Contributor

You requested a new review but it looks like there are no changes from my latest review: did you forget to push something?

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jul 17, 2025

You requested a new review but it looks like there are no changes from my latest review: did you forget to push something?

I posted replies to your comments so that is why I requested a review on my answers

Maybe just mentioning can help
Thankyou @silverweed

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.

Thanks. Have you tested that the Selector test still works properly?

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jul 18, 2025

Thanks. Have you tested that the Selector test still works properly?

Yes @silverweed
For simple.root you created it's still working perfectly fine

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jul 18, 2025

Thanks. Have you tested that the Selector test still works properly?

Yes @silverweed For simple.root you created it's still working perfectly fine

I was thinking to try this code after creating some other simple root files and test on them
But for now I think we can merge the code.

@silverweed silverweed merged commit 04b778d into root-project:dev Jul 18, 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