Skip to content
This repository has been archived by the owner on Mar 1, 2018. It is now read-only.

Updates quota limit for car renting #86

Merged
merged 1 commit into from
Oct 7, 2017

Conversation

pedrovereza
Copy link
Contributor

Fixes #62

New quota was defined as R$ 12.713,00.

@pedrovereza pedrovereza force-pushed the car-renting-quota branch 2 times, most recently from 14d148f to 382c89d Compare October 4, 2017 00:08
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.025% when pulling 382c89d on pedrovereza:car-renting-quota into 4b0fc66 on datasciencebr:master.

@jtemporal
Copy link
Collaborator

Hi @pedrovereza great PR! I have a question though:

For those reimbursements from before the new definition shouldn't we have a monthly limit still somewhere in the code? I don't think the new quota value also applies to "before monthly limit increase".

@cuducos @anaschwendler what you guys think? and better yet how to properly insert this piece of information on the code?

Copy link
Collaborator

@cuducos cuducos left a comment

Choose a reason for hiding this comment

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

Many thanks for the PR Pedro.

I agree with @jtemporal: We do not delete the old limit — as we filter cases by dates, we can use the current limit for reimbursements from mid-2017 and on, and the old limit for the period when that limit was still in use. Does that make sense? Cheers

@pedrovereza pedrovereza force-pushed the car-renting-quota branch 2 times, most recently from e08da36 to bdcf372 Compare October 7, 2017 14:36
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.025% when pulling bdcf372 on pedrovereza:car-renting-quota into 4b0fc66 on datasciencebr:master.

@pedrovereza pedrovereza force-pushed the car-renting-quota branch 2 times, most recently from e7d3ad5 to 6a5a1e3 Compare October 7, 2017 14:44
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.025% when pulling 6a5a1e3 on pedrovereza:car-renting-quota into 4b0fc66 on datasciencebr:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.025% when pulling 470da94 on pedrovereza:car-renting-quota into 4b0fc66 on datasciencebr:master.

@pedrovereza
Copy link
Contributor Author

@jtemporal @cuducos Hey, thanks for the feedback! The change requested totally makes sense, and I think I got it right now 😉

@@ -18,15 +18,15 @@ def setUp(self):

def test_predict_false_when_not_in_date_range(self):
assert_array_equal(np.repeat(False, 10),
self.prediction[[0, 1, 9, 10, 18, 19, 27, 28, 36, 37]])
self.prediction[[0, 1, 18, 19, 27, 28, 36, 37, 45, 46]])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick note here: car renting before 07/2017 is not really out of range as it falls into the previous period (04/2015 - 06/2017)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 98.028% when pulling 470da94 on pedrovereza:car-renting-quota into 4b0fc66 on datasciencebr:master.

@cuducos cuducos merged commit 3d98ee5 into okfn-brasil:master Oct 7, 2017
@cuducos
Copy link
Collaborator

cuducos commented Oct 7, 2017

Yay, many thanks, @pedrovereza : ) Great contribution o/

@pedrovereza pedrovereza deleted the car-renting-quota branch October 7, 2017 17:57
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