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

Fix Bounding Box Checks for Tiff Export #5244

Merged
merged 1 commit into from
Mar 8, 2021
Merged

Fix Bounding Box Checks for Tiff Export #5244

merged 1 commit into from
Mar 8, 2021

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Mar 8, 2021

Follow-up Bugfix for #5195

  • Correctly parse bounding box for size assertions (assume width,height,depth instead of xmax,ymax,zmax)
  • Use Long multiplication for volume to avoid int overflow
  • Remove some unused util methods in BoundingBox class
  • Add some typing to public methods in BoundingBox class

Steps to test:

  • with jobsEnabled, go to routes

  • GET http://localhost:9000/api/jobs/run/tiffExport/sample_organization/testDataset/color?bbox=50,50,50,5000,5000,5000 (should say volume too large)

  • GET http://localhost:9000/api/jobs/run/tiffExport/sample_organization/testDataset/color?bbox=50,50,50,10,10,10 (check should go through, attempt contacting jobs broker. this was broken before, as 10,10,10 was assumed to be bottom-right, which was invalid)

  • (I did test these locally)

  • CI should run through

  • Ready for review

@fm3 fm3 self-assigned this Mar 8, 2021
@fm3 fm3 requested a review from jstriebel March 8, 2021 08:30
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

LGTM 👍, just one minor nitpick, feel free to leave as-is.

@fm3 fm3 merged commit a67825c into master Mar 8, 2021
@fm3 fm3 deleted the fix-bbox-check branch March 8, 2021 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants