[MRG] update impostors, closer to original implem #228
Conversation
|
It now works on my machine, so I guess the only thing needed is a bit of refactoring, but at first sight, what do you think @bellet @perimosocordiae ? |
|
Also, the test example does not have a decreasing number of active constraints. I'll try to find an example where it's the case, also where the original impostors are not the ones at the end. This way, everything will be properly tested. |
|
So I tried on another example (more features, and more noise, to be more like what we saw in #208 , that is to say distances between points don't separate them well at first) where the number of active constraints diminishes, and it does not work anymore (the values are still similar but not exactly the same). I'll try to find why. |
|
Looks fine at first sight |
|
I just added the more robust test where the number of impostors varies and it fails this time on my machine (at iteration 21, the gradient is quite different, I checked if it could be some accumulated uncertainties somewhere but it does not seem so, since the gradient is exactly equal at iteration 20...) I'll check what's happening |
|
Did you check if this improves the results on the doc example? Overall i think not updating the impostors was problematic and not consistent with the specifications of the algorithm. We can probably improve the efficiency later but i am for merging this now (if the tests pass) |
|
Yes, it improves the result, but this time at the condition to not set the init (i.e. not set it to 'random', as before). Setting to random barely changes wrt the original image. I'll remove it in this PR. Here is the result:
|
wdevazelhes
changed the title
|
CI tests are green we can merge when we're ready |
|
Let's merge this and maybe if needed later we can make changes |
This is another attempt to update impostors in LMNN (like #223), closer to the original implementation