Draft
Merge within Tolerance
11Fbasham
Loading description...
Fundamentals
View
This comment has been reported as {{ abuseKindText }}.
Show
This comment has been hidden. You can view it now .
This comment can not be viewed.
- |
- Reply
- Edit
- View Solution
- Expand 1 Reply Expand {{ comments?.length }} replies
- Collapse
- Spoiler
- Remove
- Remove comment & replies
- Report
{{ fetchSolutionsError }}
-
-
Your rendered github-flavored markdown will appear here.
-
Label this discussion...
-
No Label
Keep the comment unlabeled if none of the below applies.
-
Issue
Use the issue label when reporting problems with the kata.
Be sure to explain the problem clearly and include the steps to reproduce. -
Suggestion
Use the suggestion label if you have feedback on how this kata can be improved.
-
Question
Use the question label if you have questions and/or need help solving the kata.
Don't forget to mention the language you're using, and mark as having spoiler if you include your solution.
-
No Label
- Cancel
Commenting is not allowed on this discussion
You cannot view this solution
There is no solution to show
Please sign in or sign up to leave a comment.
Hi,
The description still lacks some corrections (typos, mostly) and one clarification, imo:
...(..., tol=3)
, which you're not doing currently (I didn't check the sample tests, tho). That's slightly different in the way that, if you say "keyword", you should check the user actually usedtol
as argument name. If you don't care about the user changing the argument name in the function declaration, call it a "default argument". Advise: switch to "default"... x) (or use a totally meaningful name =>tolerance
, but taht's a bit verbose... x) )Cheers
Made most of the changes you recommended and added additiona random tests with small lists. Maybe I'm biased because I'm the author, but I don't see where else I can clarify what needs to be done especially given the fixed tests and the example/description. Thanks for your input and suggestions!
about the last point, maybe just add "(the same row of b may be merged withe different rows of a if it's always the best candidate according to the rules)" or somethig like that?
Added, but I think I need to get better at writing descriptions and keeping it simple ;p
:+1:
;)
There should be smaller random tests too because debugging when almost every error message is 20 lines long is very hard.
Length of lists has been dropped.
It should be stated whether all options should be taken or only one (and which one exactly), e.g. what's the result of these:
In the first example it would be [1,...] from list
b
since it has an exact match. For the second example it would be [3,...] since it minimizes the difference and is within tolerance.I re-worded the bullet point in the 'Notes' section where it's mentioned.
What about identical distances?
always the first match, so [0,X]
This info should be added to the description, and such cases must be tested in the fixed tests (and preferably sample tests).
Also what about this?
Or are such cases guaranteed to not be generated?
it would be [2,X]
I can modify the description and and fixed tests. I'm a little reluctant to change the random tests, but I could add more? The pattern here is that we're minimizing the absolute difference between list
a
and listb
in column 1. If that is minimized and the value is within tolerance, it will always pick the first value it can find. There are likely cases of this in the random tests.Edit: By the way, this is mimicking a pandas function and I've checked it's intended behaviour with this function. Both line up ;)
The random tests are alright. I've implemented everything except the "pick first closest" scenario and failed 200+ tests. But all this stuff must still be tested in the fixed tests where the input size is small, expected result doesn't change all the time, and different values are used in the last column, e.g.:
Also, it'd be good if the example in the description covered as many rules/details as possible, similar to how I rewrote the example in your other kata, and now it shows that: multiple columns are aggregated separately, rows with identical keys don't have to come one after another to be grouped, different combinations of keys are not grouped together.
That's half the fun of guessing what the hell the author wrote ;p
I'll let you know I've finished changes.
Okay, I've changed the fixed tests and implemented the two examples you gave above. Not sure about description. I'm on the fence since it lays out pretty much everything I can think of.
There're still no tests like these:
Changed. I think the user should be good to visit the test cases to answer any questions they might have. One thing about long kata descriptions is people generally don't read them and immediately jump into the coding.
(still waiting for a btach of small random tests... => trying another kind of implementation and it's passing the fixed tests but not the random ones)
EDIT: mmmh... XD Sounds like they are still a bit "too big", then.