-
Notifications
You must be signed in to change notification settings - Fork 3
Feat: created annotation handler #38
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
base: main
Are you sure you want to change the base?
Conversation
ngtools/local/handlers.py
Outdated
| LOG = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class AnnotationHandler(Handler): |
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.
I would call it TractAnnotationHandler
ngtools/local/handlers.py
Outdated
| """Handler that returns annotation data.""" | ||
|
|
||
| annotation_cache = {} | ||
| spatical_cache = {} |
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.
spatical -> spatial
(please be careful with typos, especially in code)
ngtools/local/handlers.py
Outdated
| class AnnotationHandler(Handler): | ||
| """Handler that returns annotation data.""" | ||
|
|
||
| annotation_cache = {} |
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.
We need to be careful with caches.
It seems that you never clear it. It might be better to use a cache that has a size limit (you can maybe reuse or copy this one, although we might need something a bit more advanced that puts a file back on top of the queue when used).
We should also ask Scene.unload to remove files from cache when possible.
ngtools/local/handlers.py
Outdated
| spatical_cache = {} | ||
| grid_densities = [1, 2, 4, 8] | ||
|
|
||
| def read_from_file(self, path: str) -> Tuple[np.ndarray, np.ndarray, np.ndarray]: |
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.
Make it private: _read_from_file
ngtools/local/handlers.py
Outdated
| self.headers["Content-type"] = "application/json" | ||
| self.body = json.dumps(info).encode() | ||
| return None | ||
| if self.parse_path(path) is not None: |
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.
Separate the cases with an empty line, for readability.
ngtools/local/handlers.py
Outdated
| else: | ||
| return None | ||
|
|
||
| def get_spaticals(self, path: str) -> bytes: |
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.
typo spaticals -> spatials
Also, make private: _get_spatials
ngtools/local/handlers.py
Outdated
| self.spatical_cache[trk_path] = spatial | ||
| return spatial[int(parent)][f"{a}_{b}_{c}"] | ||
|
|
||
| def get_info(self, path: str) -> dict: |
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.
Make private: _get_info
ngtools/local/handlers.py
Outdated
| raise FileNotFoundError( | ||
| f"Tract file not found: {trk_path}") | ||
|
|
||
| segments, bbox, offsets, affine = self.read_from_file(trk_path) |
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.
It seems like you read the entire file every time info is queried.
It's probably worth caching it as well.
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.
The file is cached, inside that function it caches it. unless you are saying I should cache info as well which is not a bad idea
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.
I meant that if the info file is queried twice, the trk file is going to be read again, since caching happens later in the function, no? But yes I guess it's relatd with caching the info file or not. (I am not so worried about caching the info file itself as I am with caching the trk-dependent quantities that are computed to create the info file)
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.
The end of self._read_from_file caches the segments, bbox, offsets, and affine before returning into annotation_cache.
ngtools/scene.py
Outdated
| "Cannot load local files without a fileserver" | ||
| ) | ||
| short_uri = fileserver + "/local/" + op.abspath(short_uri) | ||
| if parsed.format == "precomputed" and short_uri.endswith(".trk"): |
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.
I would actually not look for the precomputed:// hint. If the file is a trk -> use the handler.
And we might also want to include a trk:// hint for trk files that do not have the trk extension.
ngtools/local/viewer.py
Outdated
| (r"^/local/(.*)", StaticFileHandler), | ||
| (r"^/lut/([^/]+)/(.*)", LutHandler), | ||
| (r"^/linc/(.*)", LincHandler), | ||
| (r"^/ann/(.*)", AnnotationHandler), |
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.
I would use the /trk/ prefix
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.
Also, we might want to accept a protocol prefix, like in the LutHandler, in case the trk file is remote instead of local. This allows path of the form https://some/url/to/file.trk to be mapped to {fileserver}/trk/https/some/url/to/file.trk, while local files are mapped to {fileserver}/trk/file/some/path/to/file.trk.
|
I only have an issue with this option in the flowchart:
I'd prefer we only use the tracts style if we are confident the file really contains tracts data. Maybe we could look for the presence of the "orientation" property? |
balbasty
left a comment
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.
Almost there!
ngtools/local/handlers.py
Outdated
| import trk_to_annotation.utils as tau | ||
|
|
||
| # internals | ||
| from ngtools.datasources import _SizedRefreshCache |
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.
Can you move the cache classes to ngtools.utils, now that they are used in multiple modules?
ngtools/local/handlers.py
Outdated
|
|
||
| try: | ||
| folder_listing = {"path": path, | ||
| "children": ["info", "0", "1", "2"]} |
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.
Should the list of level be read from the info file?
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.
This is for the benefit of neuroglancer to make the .trk file look like a folder. I think it was needed, but I implemented this super early when I was still testing on actual neuroglancer instead of within ngtools so it might not be needed for ngtools
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.
I will double check
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.
not needed. Removing now
ngtools/local/handlers.py
Outdated
| if path in annotation_cache: | ||
| return annotation_cache[path] | ||
| if os.path.exists(path): | ||
| segments, bbox, offsets, affine = tap.load_from_file(path) |
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.
I feel like we don't really need this case, we can always use the filesystem opener (it work with local files).
| del spatial_cache[key] | ||
| for key in annotation_cache.keys(): | ||
| del annotation_cache[key] | ||
|
|
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.
We should only remove cached values that are used in this unloaded layer I think.
Thinking more about it: maybe check whether the cached data is used in any remaining layer, and only delete it if not (in case multiple layers use the same datasource).
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.
ah my bad. I don't know why but I assumed unload was to unload everything. That makes more sense though
ngtools/scene.py
Outdated
| ) | ||
| short_uri = fileserver + "/local/" + op.abspath(short_uri) | ||
| if parsed.format == "trk": | ||
| short_uri = fileserver + "/trk/local/" + op.abspath( |
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.
Have you tried this case? I am thinking it should be /trk/file/ instead of /trk/local/ (unless fsspec understands local as an alias for file)
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.
I have tested this, but I just had a hard coded check to ignore local protocol. I forgot that file protocol was a thing
|
|
||
| return info | ||
|
|
||
| def _get_transform(self, path: str) -> np.ndarray: |
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.
Is this used anywhere?
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.
yes on line 59 of this file
…ales/ngtools into james/annotation_handler
Created an annotation handler that automatically emulates .trk files as precomputed annotation folder
tracts will be loaded based on this flowchart
flow chart for how to handle tracts:
format is trk -> use handler
file to be loaded ends in .trk, .tck or .tracts and format is empty or precomputed and layer is not tractsv1 -> use handler
layer is tracts or tractsv2 -> load premade precomputed annotation with tract styles
layer is tractsv1 -> load tracts as skeleton
format is precomputed and type is line annotations -> load premade precomputed annotation with tract styles
format is precomputed -> load premade precomputed format with no styles (not implemented yet)
otherwise -> assume its not tracts