Conversation
9b5600c to
20e79eb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1311 +/- ##
==========================================
- Coverage 87.35% 86.87% -0.49%
==========================================
Files 24 24
Lines 2981 3176 +195
==========================================
+ Hits 2604 2759 +155
- Misses 377 417 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Coverage looks OK. Some edge cases in the unit tests which we could add later, and main coverage can be ignored for now as we don't have models in yet. |
|
Pausing review for a moment while I check some naming consistencies with utilities. |
|
@jveitchmichaelis can you either take me off request, or take off draft, trying to organize the issue queue. |
20e79eb to
5d38915
Compare
|
Minor change here - the density map is normalized to sum to the object count for each class. |
|
@bw4sz this is good for review now, ignoring the pre-commit.ci failure. |
bw4sz
left a comment
There was a problem hiding this comment.
I'm happy with the /src. I always am unsure about the amount of tests. For example, do we need flip tests? I think it's fine and they are pretty cheap to run. I'm going to approve, but something for others to weigh in on as we think about the size of the codebase, which I sense is going to swell a lot over the next year.
|
Happy to merge once the pre-commit is solved. |
|
Agree. I added the flip test to make sure that we have coverage for augmentations, since we haven't really tested them with points. Dataset tests are at least very fast, as you say. |
|
Please rebase main and that should be fixed. |
5d38915 to
d3a4a53
Compare
|
Good to go when the CI passes |
|
The #1343 made a tiny conflict, that I think you can handle right in GitHub. Then I will merge. |
d3a4a53 to
c7165a7
Compare
|
|
c7165a7 to
116544b
Compare
116544b to
18fe41b
Compare
|
All good, first rebase had a small conflict |
Description
This PR adds a keypoint dataset that returns either (x, y) coordinates or a 2D density mask as target. The dict entry
labelsis consistent withtransformerssemantic segmentation models. This should cover the majority of the models that we might want to train. The structure is essentially the same as BoxDataset.The diff is better than it looks. I've pulled out some duplicate logic for the box dataset and made base class which will be used for polygons as well. Most of the additions are in the test suite.
Models now have a
taskattribute which is used to load the correct dataset type (e.g. all current models havetask = box). I think this is cleaner than having the user specify this via config, since it should be obvious by the choice of architecture.Training is not supported, but I'd like to add that separately to reduce review load.
Related Issue(s)
#809
Supersedes #1182 as features from there are migrated.
AI-Assisted Development
Some suggest tests and refactoring.
AI tools used (if applicable):
Claude Code, Gemini