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

Clean up and improve how mismatching resolutions are detected #4936

Merged
merged 4 commits into from
Nov 12, 2020

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Nov 11, 2020

The nightly test fails in one test case due to "mismatching resolutions". However that failure was a false positive, which is caused by the fact that we keep the original resolutions per layer instead of storing the "densified version". The code had problems with a dataset where the resolutions were like this:

  • mag 1, mag 2, mag 4, mag 8
  • mag 32

The old code densified the resolutions (i.e., padded all the missing resolutions for the second layer) and assumed that the longest resolution list covers all resolutions. The new code constructs a union of all resolutions and checks that there are no mismatches while doing so.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • see new automated tests
  • execute nightly

Issues:


@philippotto philippotto self-assigned this Nov 11, 2020
@philippotto
Copy link
Member Author

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice fix and it makes sense to me. Impressive how well the longest-list-covers-all-resolutions heuristic hold up 🙈 Thanks for adding the tests and for confirming the fix by running the screenshot tests 🎉

@philippotto philippotto merged commit 8613e89 into master Nov 12, 2020
@philippotto
Copy link
Member Author

Impressive how well the longest-list-covers-all-resolutions heuristic hold up see_no_evil

I think (but maybe I'm missing something), the previous logic was quite "precise" (but not elegant 😆), since the resolutions were always "densified" (starting at mag 1 until the maximum) in the initialization. Therefore, essentially the range from "mag 1" to "highest mag" was used, which is probably equivalent to the new way (building the union and then densifying it).

@philippotto philippotto deleted the fix-resolution-assertion branch November 12, 2020 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants