Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Array loaderbase merge #121

Merged
merged 6 commits into from
Apr 4, 2023

Conversation

jperez999
Copy link
Collaborator

This PR merges the array loader into the LoaderBase class. There is no need to have two different classes as all inheritance derives from one set of base logic (array based dataloading) so now we can merge the array and loader base.

Copy link
Contributor

@karlhigley karlhigley left a comment

Choose a reason for hiding this comment

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

This looks good! Always nice to see a red diff 👍🏻

(Seems like there's a minor issue to work out in the JAX dataloader though)

@@ -528,26 +589,6 @@ def _process_batch(self, tensors):

return X, labels

def _pack(self, gdf):
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is still used in the JAX dataloader

@karlhigley karlhigley merged commit 1ace256 into NVIDIA-Merlin:main Apr 4, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants