Extending annotations by adding units #59
Conversation
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 96.90% 96.93% +0.03%
==========================================
Files 16 16
Lines 1294 1307 +13
==========================================
+ Hits 1254 1267 +13
Misses 40 40 |
|
@iranroman once we merge this you can start with #58 |
There was a problem hiding this comment.
@magdalenafuentes nice work! Added some comments, happy to discuss to iron out quickly. tnx!
soundata/annotations.py
Outdated
| intervals_unit="s", | ||
| labels_unit="open", | ||
| confidence_unit="binary", |
There was a problem hiding this comment.
I worry that if these values have defaults people will miss/ignore them and that will leads to bugs (e.g. dataset annotated in milliseconds but the interval unit is set to s.
@magdalenafuentes thoughts about not having default values here and forcing the user to set them explicitly?
There was a problem hiding this comment.
Yeah I agree.. If we enforce it then we have to edit all the loaders. I'm ok doing that because we don't have as many loaders yet, but in the future it would be nice to think of a way of introducing changes without having to manually edit all files. I haven't really found a workaround for it..
There was a problem hiding this comment.
Yes, you raise a good point (about updating loaders). But hopefully the API (especially for mandatory fields) doesn't change very often (changing mandatory fields would be a breaking change so ideally we avoid it as much as possible), so I don't anticipate us having to make these updates very often at least.
If you're OK with it, I would vote for making these mandatory.
There was a problem hiding this comment.
I agree on making them mandatory. About the breaking change, I super agree, but the changes I'm talking about are not always breaking changes, we can break things under the hood without the user noting and those usually need an update of loaders, which is not ideal. Anyway, just dropping that idea here so we keep it in mind for the future, let's see if we can come up with a strategy to avoid those
…soundata into fuentes/annotations_extensions
|
@justinsalamon I made the changes we discussed, let me know how it looks. Also, I added a commit with a proposal to change the |
There was a problem hiding this comment.
@magdalenafuentes looks awesome. mentioned a couple of tiny nits. 🚀
|
Be sure to bump the version after you merge and make a new release :) |
Following the discussion in #58 I think we should follow ideas in
mirdataand addunitsto the annotations, so they become "reusable", e.g. we use the same annotation type for annotations withazimuthindegreesorradiansby simply indicating theunits.Note that I introduced the
unitssome default values, in particular forconfidence_unit="binary",labels_unit="open"andintervals_unit="s". I think this is correct for all datasets that we have now but please keep this in mind when reviewing the code because it could introduce silent bugs. If you prefer I can make the units mandatory but that means having to modify all tests and loaders.