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

Switch to Bottom as upper bound for Quantizer Rect #2549

Closed
wants to merge 1 commit into from

Conversation

mrange
Copy link

@mrange mrange commented Oct 10, 2023

Hi.

I was trying out Quantizer with custom rects. I couldn't get it to work and checked the code. To me it seems to be a typo where the interest.Height was used rather than interest.Bottom. The x-dimension interest.Left and interest.Right is used which makes sense to me.

I did the change and it worked for me but then when I tried running the tests I got multiple image comparison failures.

So perhaps this is the intended behavior and I misunderstood the API. Or it is a good fix and oracles should be regenerated (but I don't know how).

I don't really know what is the right choice here so I thought I create a draft pull request even though the tests fails and you can tell me what do or just close it if it works as intended.

Regards,.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mrange
Copy link
Author

mrange commented Oct 11, 2023

Read the code a bit closer today and it seems I need to think it through one more. Closing for now. Still think there's something wrong with original (but could be wrong)

@mrange
Copy link
Author

mrange commented Oct 11, 2023

Thinking...

@mrange mrange closed this Oct 11, 2023
@JimBobSquarePants
Copy link
Member

@mrange There is an issue, but a fix needs to be applied in two places. I'm working on the fix now.

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.

None yet

3 participants