Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Conversation

@rrader
Copy link
Contributor

@rrader rrader commented Mar 19, 2019

BATCH SCORING PULL REQUEST

This is a pull request into a public repository for Batch Scoring script maintained by DataRobot.

RATIONALE

The structure of datarobot_batch_scoring.api_response_handlers.pred_api_v10.format_data function makes it difficult to understand the code and add new columns.

CHANGES

  • format_data was refactored in favor of generators approach.
  • now all columns are independent functions which return headers and generator of rows

TESTING

  • unit tests should confirm refactoring didn't break the contract

@devexp-slackbot
Copy link

@coveralls
Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage increased (+0.06%) to 84.015% when pulling 0ffe5ea on rrader/PRED-2176-refactoring into 83cd10f on master.

row_generator: iterator
"""
headers = []
single_row = result_sorted[0]
Copy link

Choose a reason for hiding this comment

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

Will there always be at least one item in result_sorted? Otherwise this could raise an IndexError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's a check that forbids sending empty dataset for scoring much earlier
https://github.com/datarobot/batch-scoring/blob/master/datarobot_batch_scoring/reader.py#L255

Copy link

@chrj chrj left a comment

Choose a reason for hiding this comment

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

Very nice refactoring. LGTM.

@rrader rrader merged commit c7f9897 into master Mar 20, 2019
@rrader rrader deleted the rrader/PRED-2176-refactoring branch March 20, 2019 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants