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

[Torchscript] Adds NaN handling to preprocessing modules #2179

Merged
merged 17 commits into from
Jun 24, 2022

Conversation

geoffreyangus
Copy link
Contributor

This PR adds NaN handling in Torchscript to the majority of input features. This is tested by running preprocessing in both the LudwigModel and its torchscript equivalent and verifying the parity of the outputs.

There are four input features where NaN handling is left unimplemented:

  • Image feature. The default missing strategy value is BACKFILL, which is unintuitive in a real-time inference setting.
  • Audio feature. The default missing strategy value is BACKFILL, which is unintuitive in a real-time inference setting.
  • Vector feature. Vanilla LudwigModel preprocessing does not implement a missing value strategy.
  • Date feature. The default missing strategy, which uses datetime.now(), is not torchscriptable.

@github-actions
Copy link

github-actions bot commented Jun 21, 2022

Unit Test Results

       6 files  ±  0         6 suites  ±0   2h 16m 6s ⏱️ -28s
2 849 tests +  9  2 815 ✔️ +10    34 💤 ±0  0  - 1 
8 547 runs  +27  8 441 ✔️ +28  106 💤 ±0  0  - 1 

Results for commit e4e5a25. ± Comparison against base commit 36787ba.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@justinxzhao justinxzhao left a comment

Choose a reason for hiding this comment

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

LGTM

@geoffreyangus geoffreyangus merged commit f54818e into master Jun 24, 2022
@geoffreyangus geoffreyangus deleted the ts-add-missing-values branch June 24, 2022 19:30
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