Joe/feat 806 #863
Joe/feat 806 #863
Conversation
| layer = build_annotation_layer(path, mode="read") | ||
| response = [] | ||
| for line in layer.read_in_bounds(index): | ||
| for line in layer.read_in_bounds(bbox, strict=True, annotation_resolution=resolution_vec): |
There was a problem hiding this comment.
Should we decouple the bbox specification resolution from the resolution we want the resulting annotations to be in?
There was a problem hiding this comment.
I'd say that's up to the users of the web API (your team in Ukraine, I guess). Most of the time I would expect them to be the same; it's also easier to avoid mistakes if you don't have to keep two different resolutions in mind. But if they have some reason to need them separate, we can easily do that.
| @@ -65,8 +67,8 @@ async def add_multiple( | |||
| resolution: Annotated[tuple[float, float, float], Query()], | |||
There was a problem hiding this comment.
Same for the write -- should we allow the start and end coord to be given in resolutions that are not the same as the ones used for annotations?
There was a problem hiding this comment.
Same answer. I don't have strong feelings about it, but slightly prefer to have only one resolution.
| @@ -57,7 +58,9 @@ def __init__(self, line_id: int, start: Sequence[float], end: Sequence[float]): | |||
| :param start: A tuple of three floats representing the start coordinate (x, y, z). | |||
| :param end: A tuple of three floats representing the end coordinate (x, y, z). | |||
|
|
|||
| Coordinates are in units defined by "dimensions" in the info file. | |||
There was a problem hiding this comment.
btw, is there a reason we're not using attrs here like we do in the rest of the codebase?
There was a problem hiding this comment.
Just that I'm still not super familiar with it, or the reasons one might want to use it.
There was a problem hiding this comment.
It's just a cleaner more modern way to use python that reduces boilerplate and comes with a lot of free gooddies
There was a problem hiding this comment.
We are trying to use it for all classes except for the ones that inherent from pytorch.nn.module (pytorch does some incompatible black magic)
There was a problem hiding this comment.
OK. I know you were eager to get this to your web team, so it's up to you whether you want to hold this until I can do that refactoring (probably tomorrow), or merge this as-is and I'll do another little PR to add attrs.
zetta_utils/internal
Outdated
There was a problem hiding this comment.
does the internal need to update here or is this an accident?
There was a problem hiding this comment.
Dangit. That's an accident again. To check my code against the pre-commit hooks I have to commit them multiple times, and when there are multiple files, the easiest way to do that is commit -a. And I forget that, even though I didn't touch internal, git sees it as changed (I guess because I was required, by another pre-commit hook, to switch internal to main). So, that must be how this keeps happening.
I will try to undo that part.
There was a problem hiding this comment.
Oh! Actually, no, in this case I do need to update internal as well (to track changes to the AnnotationLayer API). I'll make a separate PR for that.
There was a problem hiding this comment.
https://github.com/ZettaAI/internal/pull/67
Not sure whether/how to remove the change to internal in this case, or make it point at the right thing. But maybe it doesn't matter, since to get everything tidied up we will need to merge internal PR#67, and then pull that into main, and make another mini-PR on main to update internal, right?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 142 142
Lines 6041 6066 +25
=========================================
+ Hits 6041 6066 +25 ☔ View full report in Codecov by Sentry. |
JoeStrout
force-pushed
the
joe/feat_806
branch
2 times, most recently
from
4b341d7 to
6d79e8c
Compare
JoeStrout
force-pushed
the
joe/feat_806
branch
from
6d79e8c to
cb4a4b3
Compare
supersergiy
force-pushed
the
joe/feat_806
branch
from
cb4a4b3 to
bcd702f
Compare
JoeStrout
force-pushed
the
joe/feat_806
branch
from
bcd702f to
22228f5
Compare
Adds
clearing_bboxparameter to Annotation.write_in_bounds, and uses this in the PUT web API to replace only the annotations in the specified region.Also shores up how resolution is handled — reading and writing now allow the caller to specify the resolution at which the line coordinates should be interpreted (converting to/from the underlying file resolution as needed).