-
Notifications
You must be signed in to change notification settings - Fork 1
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
CSV download #92
CSV download #92
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I saw this too - it's likely a bug on |
an example csv is on sheet 2 here: https://docs.google.com/spreadsheets/d/1akqPHvt_Yu7lZ7L6E_2bxM7D0u6lYDpiLheouJ_l3Kk/edit?gid=1909503628#gid=1909503628 I think there's something wrong with the data, but still looking into it |
Can you grant access to that sheet? |
done! sorry!
I don't think so. The values seem to be trackIds, just not the right ones. Maybe I'm putting them in the data rows instead of columns? |
update: I tried switching the rows/columns and that was very obviously wrong. All of the tracks were listed as having no parentTrackId. |
I'm going to try to look at this more closely today. |
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.
Still need to finish looking at everything, but generally looks good. There's seems one bug maybe related to the track geometry changes that needs to be addressed before this can be merged though.
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.
Generally looks good and I was able to export some tracks and read them in napari, where all looked okay to me.
I have a couple questions and nitpicks, and also interested about some of @andy-sweet's comments.
min length of points should be 6 Co-authored-by: Andy Sweet <[email protected]>
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.
Looks good to me and the feature is a great to tie this back to the napari plugin. Some nice improvements in the CSV to Zarr converter too! I have one optional suggestion/simplification, but it's non-blocking.
Given that Ashley has more experience with the three.js data structures here, you should wait for approval from him before merging!
simplify csv conversion function Co-authored-by: Andy Sweet <[email protected]>
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.
LGTM! Thanks for taking this one on.
Resolves #78
TODO:
write some testsI ended up testing manually - should these be automated?To test:
run the script to generate a new zarr bundle and serve it locally
NOTE: We will need to update the data on S3 to include the data field. There will be downtime if this is not done first.