-
Notifications
You must be signed in to change notification settings - Fork 10
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
Bounding boxes example #312
Conversation
Quality Gate failedFailed conditions |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
=======================================
Coverage 99.78% 99.78%
=======================================
Files 14 14
Lines 932 932
=======================================
Hits 930 930
Misses 2 2 ☔ View full report in Codecov by Sentry. |
b02435f
to
a1d17b3
Compare
Need resolving |
hey, thanks for having a look! From the issues flagged by sonarcloud:
I think for clarity it makes sense to keep the duplicate lines as is and not define a separate variable, but lmk if you disagree. I cannot retrieve the reports from previous tutorials anymore (examples here and here) but it seems in those cases we skipped three issues too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sfmig, this is a great example! I left a bunch of comments, many of them nitpicky and/or subjective.
My two substantial points are:
- I think we should change the name and/or tagline of this example. I don't think that "reindexing" means much to most users
- we should rely on selecting the data with
sel
andisel
as much as possible, and avoid numpy-style indexing (which is contingent on a certain dimension order).
Regarding the sonarclous issues, I agree that you can ignore them in this case. We don't mind some repetition in the examples (it's often a good thing, pedagogically). |
Co-authored-by: Niko Sirmpilatze <[email protected]>
4344ab4
to
9a8f74e
Compare
…lors consistent with the rest of the notebook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 1 comment @sfmig, and after that it's good to merge!
I really like the new plots you've added now.
Quality Gate passedIssues Measures |
Rebase after #313 is merged
Description
What is this PR
Why is this PR needed?
To demo the bounding boxes' dataset.
What does this PR do?
References
Closes #247.
Rebase after #313 is merged
How has this PR been tested?
Docs build locally and test pass locally and in CI.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
No.
Checklist: