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.

Sign up for GitHub

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

Jump to bottom

Avoid int-to-float conversions #1098

Merged
merged 10 commits into from
Nov 15, 2019
Merged

Avoid int-to-float conversions #1098

merged 10 commits into from
Nov 15, 2019

Conversation

adriangonz
Copy link
Contributor

@adriangonz adriangonz commented Nov 11, 2019 •
edited
Loading

Fixes #948.

Changelog

  • Serialise as Long if Double/Float's decimal part is empty
  • Add tests around JSON to proto conversion and viceversa

TODO

  • Document int-to-float behaviour after this fix and potential pitfalls
  • Consider if we need to force explicitly dtype=float on Python wrapper

@adriangonz adriangonz changed the title WIP : Avoid int to float conversions WIP : Avoid int-to-float conversions Nov 11, 2019
@axsaucedo
Copy link
Contributor

Nice one @adriangonz ! Definitely a step towards the right direction
/test integration

@adriangonz
Copy link
Contributor Author

/test this

@adriangonz
Copy link
Contributor Author

/cc @cliveseldon @axsaucedo

@adriangonz adriangonz changed the title WIP : Avoid int-to-float conversions Avoid int-to-float conversions Nov 12, 2019
@adriangonz
Copy link
Contributor Author

/test integration

1 similar comment
@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

NIce all integration tests pass!
/approve

@axsaucedo
Copy link
Contributor

/lgtm

@seldondev seldondev added the lgtm label Nov 15, 2019
@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: axsaucedo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@axsaucedo axsaucedo merged commit df528ea into SeldonIO:master Nov 15, 2019
@adriangonz adriangonz mentioned this pull request Nov 19, 2019
Closed
@axsaucedo axsaucedo removed their assignment May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axsaucedo axsaucedo Awaiting requested review from axsaucedo

@ukclivecox ukclivecox Awaiting requested review from ukclivecox

Assignees
No one assigned
Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modify engine's Proto Value to JSON conversion to avoid int-to-float conversions in REST requests
3 participants
@adriangonz @axsaucedo @seldondev