Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Landmark bugs #1745

Merged
merged 7 commits into from Oct 8, 2019
Merged

Landmark bugs #1745

merged 7 commits into from Oct 8, 2019

Conversation

michaz
Copy link
Member

@michaz michaz commented Oct 7, 2019

Minimal fix for the two failing tests in #1687.

The first was already discussed (missing setter for the 'fallback' approximator).

The second one was due to an incorrect treatment of "minned out" "delta" weights. See comment in code.

@michaz
Copy link
Member Author

michaz commented Oct 7, 2019

I understand you are already investigating creating an extra factor for the delta weights, doing performance tests etc., but IMO step one is maintaining correctness, in this case doing the right thing when a delta weight is minned out. Then, step two is seeing how we can make sure this happens less often (if necessary).

I also removed the setMaxWeight from the test. PrepareLandmarks guesses this value by itself, and this must always work, i.e. it must guess in the right direction.

@michaz
Copy link
Member Author

michaz commented Oct 7, 2019

I think (?) one issue here was the test -- there even was a "setGet"-test, but it was rather technical and white-box -- it was basically testing that the bits would be twiddled in the same way the code says they would be twiddled. :-) But the important property -- that the remaining weight must never be over-estimated -- was not tested.

Copy link
Member

@karussell karussell left a comment

Choose a reason for hiding this comment

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

Thanks!

@karussell karussell merged commit e1d5452 into master Oct 8, 2019
@karussell karussell added this to the 1.0 milestone Oct 8, 2019
@karussell karussell added the bug label Oct 8, 2019
@easbar easbar mentioned this pull request Oct 9, 2019
2 tasks
@michaz michaz deleted the landmark_bugs branch April 22, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants