Skip to content
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

Renaming ProcessList to PrepareFeatures #992

Merged
merged 9 commits into from
Feb 21, 2023

Conversation

gabrielspmoreira
Copy link
Member

Goals ⚽

This PR renames ProcessList to PrepareFeatures, so that it is more generic as it currently prepares not only list features but also scalar features.

Implementation Details 🚧

Testing Details 🔍

  • The tests were changed to match the new class name
  • The sample_batch() now has a prepare_features arg as a replacement for process_list
  • For some tests I enabled sample_batch(..., prepare_features=True) to avoid calling ListToRagged() directly

@gabrielspmoreira gabrielspmoreira self-assigned this Feb 17, 2023
@gabrielspmoreira gabrielspmoreira added the enhancement New feature or request label Feb 17, 2023
@github-actions
Copy link

Documentation preview

https://nvidia-merlin.github.io/models/review/pr-992

@gabrielspmoreira gabrielspmoreira added this to the Merlin 23.03 milestone Feb 21, 2023
@gabrielspmoreira gabrielspmoreira merged commit 95f48f6 into main Feb 21, 2023
edknv pushed a commit that referenced this pull request Feb 24, 2023
* Fixed error that was causing the broadcasted context feature to have fixed size first dim in graph mode and not being compatible with the ragged sequential features

* Enforcing non-list (scalar) features to be 2D (batch size,1) if 1D or with last dim undefined (which happens in graph mode)

* Making Continuous support_masking=True (to cascade mask)

* Changing BroadcastToSequence to fix some issues and simplify the masking

* Fixed tests

* Fixed test

* Renaming ProcessList to PrepareFeatures, as it nows also prepares not only list features but also scalar features

* Fixed tests
sararb pushed a commit that referenced this pull request Feb 28, 2023
* Fixed error that was causing the broadcasted context feature to have fixed size first dim in graph mode and not being compatible with the ragged sequential features

* Enforcing non-list (scalar) features to be 2D (batch size,1) if 1D or with last dim undefined (which happens in graph mode)

* Making Continuous support_masking=True (to cascade mask)

* Changing BroadcastToSequence to fix some issues and simplify the masking

* Fixed tests

* Fixed test

* Renaming ProcessList to PrepareFeatures, as it nows also prepares not only list features but also scalar features

* Fixed tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants