-
Notifications
You must be signed in to change notification settings - Fork 34
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
new atlas file added - for cat #403
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ount for structure id
…irst line root to df
… create region mesh
…going to try and extract annotations from vtk files provided with data
… locations, will endeaver to tidy
… text and functions
|
Thanks, @HenryCrosswell - I've had an initial look and I'd say we are getting there. Some thoughts before our chat on Thursday:
Yes, we would like to have a simple check for this, but we don't yet - or it's quite manual and not documented.
Yes, this happens, but it's good to tidy. I suggest relying on the
From an initial look, it seems to me that you are generating the hash again each time you download the files (is that right?). The central idea of
The idea of these scripts is that they can run on any computer, so no, you can't expect that file to exist: could we download the original data, and modify directly, using Python?
Yes, this is tricky - sorry. This is partly because for a while in the past BrainGlobe didn't have much developer time and partly because we are currently changing things. |
…e, can change back into csv if necessary
Hey @alessandrofelder found some time to add the hard-code, let me know if you have any issues :) |
Thanks a lot @HenryCrosswell I can see a few things that need to be addressed next:
|
…ome docstrings and removed a redundant variable name
Hey @alessandrofelder sorry for the delay! But i got the updated code working within napari for me, if you wanna give it a go! I fixed the downscaling issue which seemed to have fixed the mesh placement in my version, but I'm unsure if it'll fix for you. |
Thanks @HenryCrosswell - I'll have a go and get back to you by Friday! |
Hey @HenryCrosswell There is one remaining issue I'd like your thoughts on
There are two further small issues that I can fix when we've solved the mesh conversion.
|
Hey @alessandrofelder thanks for the support, I'll try and address these comments this week and (hopefully) provide some fixes. As for the subregions question, it does download and save it locally on my desktop, but it might be a path issue that I'll have a look into. |
OK! Maybe I just couldn't find them late last night 😅 this is the VTK files? Do you get |
Oh sorry no, the function at line 317 - reads vtk files then transfers them into and saves as obj |
…le. Also updated some variable names and docstrings to be more accurate
Heyo @alessandrofelder ! The obj and vtk files are saved locally within the catlas files at your home dir, the obj files are within the folder 'meshes', whereas the vtk files, which are used to create the obj files, are within the temporary folder download_dir (which gets removed at the very end of the script). As for the ASCII, I'm not sure as to a fix, I would have thought the pre-commits would've caught anything that could corrupt the README. Let me know how if you have any suggestions to fix the ASCII issue, and if you have any other suggestions for the code! |
Before submitting a pull request (PR), please read the contributing guide.
Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)
Description
What is this PR
Why is this PR needed?
Becuase i would like this file to be added to the list of atlases
What does this PR do?
Reformats a feline atlas to make it usable to the brainglobe toolbox
References
Please reference any existing issues/PRs that relate to this PR.
How has this PR been tested?
Please explain how any new code has been tested, and how you have ensured that no existing functionality has changed.
I have been unit tested it along the way, I have used the function wrapup_atlas_from_data and it has worked.
Is this a breaking change?
N/A
If this PR breaks any existing functionality, please explain how and why.
Does this PR require an update to the documentation?
If any features have changed, or have been added. Please explain how the documentation has been updated (and link to the associated PR). See here for details.
Checklist: