Fix corner case in make_match_count_matrix() - #2191
Conversation
Add symetric option and propagate in SymmetricSortingComparison/GroundTruthComparison
for more information, see https://pre-commit.ci
…keinterface into fix_compute_matching_v4
alejoe91
added
bug
zm711
left a comment
zm711
left a comment
There was a problem hiding this comment.
Just a couple docstring suggestions
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
Great that you solved this, @samuelgarcia .?
You can also uncomment the stuff here if you want:
It is passing locally for me.
| num_units_train2, | ||
| delta_frames, | ||
| ): | ||
| """ |
There was a problem hiding this comment.
Why do we need to remove the docstring here? It is great that you added the other (my bad!) it here but is not like we are paying real state for docstrings here.
There was a problem hiding this comment.
Sorry, for me it was better for the higher function. This is why I moved it.
Having docstring here for function nested in another one will be never generated in the doc nor directly accessible. We can put it back maybe maybe not.
There was a problem hiding this comment.
That's a good point that I did not consider, but I have two points:
- It is still useful for the developer reading the code.
- I am taking your point that we should modify the way we are doing the numba functions here seriously. When we do that, I am expecting that we would be able to expose the docstrings again. And for that time, I would not like to have to urge the commit history to fetch it back.
Given that is costing nothing, would you mind leaving it just as it is? We can discuss this after the release and I can myself go and create another PR to remove if you want it. But I don't want to get this discussion get in the way of the release and I don't want to lose the docstring to the commit history either.
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
Yeah. Actually at least leave the arguments of the docstring if you are very adamant about real state.
I put a lot effort writing that docstring
Agreed. It's super helpful to at least have arguments and returns in a docstring. I understand not wanting duplication or wanting to reduce lines, but knowing the ins and outs will help with development, but also with users sending in bug reports. Recently there was an issue with a function not accepting tuples when it's partner function only output tuples so knowing there was that disconnect would have found the issue earlier. |
|
@h-mayorquin : sorry for the removal of the docstring. For me it was an internal function. Maybe we can put doc everywhere. |
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
for more information, see https://pre-commit.ci
…keinterface into fix_compute_matching_v4
for more information, see https://pre-commit.ci
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
This is fine by me.
My final comment (feel free to ignore it) is that maybe the ensure_symmetry functionality should be in the make_agreement_scores as this function can't never really be symmetric and it makes more sense over there which is where you need the scores to be symmetric.
…keinterface into fix_compute_matching_v4
Co-authored-by: Heberto Mayorquin <h.mayorquin@gmail.com>
4 participants
Uh oh!
There was an error while loading. Please reload this page.