Skip to content
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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

HenryCrosswell
Copy link

@HenryCrosswell HenryCrosswell commented Sep 3, 2024

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

  • Bug fix
  • Addition of a new feature
  • Other

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:

  • [x ] The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

HenryCrosswell and others added 22 commits September 3, 2024 17:38
…going to try and extract annotations from vtk files provided with data
@HenryCrosswell
Copy link
Author

@alessandrofelder

  • I have gotten wrap-up to not spit up errors and files have been saved but not sure how to double check those files.
  • I don't like how I problem solved the process, I would keep old code around in case I needed it again, making my script cluttered.
  • Still a bit confused as to how pooch works and I'm pretty certain i haven't utilised it fully.
  • I'm not sure whether I'm allowed to add my own files or if it was just based of this one script so no external scripts were used to run tests, just print statements to make sure outputs were as expected. On this I've had to add a file in with the workflow, see line 323, is this okay?
  • Got very confused with using the documentation, the template file as well as other peoples premade scripts as reference. It was all quite contradictory in places.

@alessandrofelder
Copy link
Member

Thanks, @HenryCrosswell - I've had an initial look and I'd say we are getting there. Some thoughts before our chat on Thursday:

I have gotten wrap-up to not spit up errors and files have been saved but not sure how to double check those files.

Yes, we would like to have a simple check for this, but we don't yet - or it's quite manual and not documented.

I don't like how I problem solved the process, I would keep old code around in case I needed it again, making my script cluttered.

Yes, this happens, but it's good to tidy. I suggest relying on the git history if you need to look at old code!

Still a bit confused as to how pooch works and I'm pretty certain i haven't utilised it fully.

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 pooch is that you get the hash once (manually) and then check that it matches the (automatically) downloaded file.

On this I've had to add a file in with the workflow, see line 323, is this okay?

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?

Got very confused with using the documentation, the template file as well as other peoples premade scripts as reference. It was all quite contradictory in places.

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.

@HenryCrosswell
Copy link
Author

Hey @alessandrofelder found some time to add the hard-code, let me know if you have any issues :)

@alessandrofelder
Copy link
Member

alessandrofelder commented Oct 11, 2024

Thanks a lot @HenryCrosswell
I managed to run this locally and it wrote files in the right format, which is good 🎉 this is very close!

I can see a few things that need to be addressed next:

  • It looks like the values in the template image will need to be rescaled. Otherwise the conversion to uint16 in the wrapup function will garble the values. There is an example code snippet in the axolotl packaging code for this.
  • We need some code to make a mesh of the root, probably best by thresholding the annotations and meshing the mask, following the same process we use to create meshes for atlases that don't provide them.
  • It looks like the meshes are not quite in the right place? You will see this when you validate the atlas as discussed yesterday.

…ome docstrings and removed a redundant variable name
@HenryCrosswell
Copy link
Author

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.

@alessandrofelder
Copy link
Member

Thanks @HenryCrosswell - I'll have a go and get back to you by Friday!

@alessandrofelder
Copy link
Member

Hey @HenryCrosswell
I've had a go at running this on my side. It's looking good, and very close.
The root mesh is in the right place for me too 🎉 well done!

There is one remaining issue I'd like your thoughts on

  • it looks to me like it downloads the VTK meshes of the subregions (maybe?)
  • but I can't find them locally
  • and it doesn't seem to write them to .obj (for me?)
    Do you have any insight into this?

There are two further small issues that I can fix when we've solved the mesh conversion.

  • the pooch registry for the vtk meshes does not persist across machines
  • there's a weird ASCII encoding error when writing the readme for me on both Mac and Linux
    (Insights/thoughts about these welcome too, but don't worry about it too much)

@HenryCrosswell
Copy link
Author

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.

@alessandrofelder
Copy link
Member

As for the subregions question, it does download and save it locally on my desktop,

OK! Maybe I just couldn't find them late last night 😅 this is the VTK files? Do you get .obj files locally?

@HenryCrosswell
Copy link
Author

Oh sorry no, the function at line 317 - reads vtk files then transfers them into and saves as obj

@HenryCrosswell
Copy link
Author

Heyo @alessandrofelder !
So i think the pooch registry is going to have to be solved a similar way as the hardcoded abbrev names. I have seperated the hashes from the file paths and have saved them locally outside of the working dir. If you run the code twice it should pick up the cached values from the first run through. Just as a proof of concept that it can run using the pooch hashs, so if theyre transfered with the zip it should be fine!

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!

@HenryCrosswell
Copy link
Author

Yo @alessandrofelder it should be all good now! (fingers crossed)

@alessandrofelder alessandrofelder marked this pull request as ready for review February 3, 2025 11:34
Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HenryCrosswell
This looks mostly ready. There seems to be one major issue around how the mesh files are named. They need to be named after their structure id. E.g. region 7 should have a corresponding 7.obj file, but it's currently region name bar the last four characters?

@HenryCrosswell
Copy link
Author

Hey @alessandrofelder should be all fixed now, I wasn't quite sure of the naming convention when i refactored the code, mb

@alessandrofelder
Copy link
Member

Thanks, @HenryCrosswell - this passes our validation so is correctly formatted. Unfortunately, however, I noticed a bug at visual inspection: I think the meshes don't overlap with the annotations - I can try to fix this... this will have something to do with the coordinate system the original vtk files are. As the expert about this data, can you think of any info/place where there might be info about the coordinate space of the vtk meshes?

@alessandrofelder
Copy link
Member

E.g. adding the mesh for region "GI" shows it outside the brain in brainrender-napari:
image

@HenryCrosswell
Copy link
Author

HenryCrosswell commented Feb 6, 2025

Hey @alessandrofelder im not sure why this is happening, the validation on napari looks fine in my view
Screenshot 2025-02-06 155236

are you adding in the GI point after? the point location should be saved in the vtk file so I can only think that the downsampling of the image could affect the location as to which the vtk files are associated.

@alessandrofelder
Copy link
Member

It works well for the root mesh, you're right (because we make the root mesh ourselves, I think?). But in the validation, if you visualise any other subregion mesh (which is based on a vtk file) it should match the equivalent subregion in the annotation image and it doesn't (for me, I think?)

You can visualise subregion meshes by their acronym (e.g. "GI"), with the extra line

napari_atlas.add_structure_to_viewer("GI")

@alessandrofelder
Copy link
Member

downsampling of the image could affect the location as to which the vtk files are associated.

I don't think we downsample the image? My guess is that the meshes are in a slightly different coordinate space (e.g. BrainGlobe puts point 0,0,0 at the anterior, superior, right corner of the image, and maybe the original data assumes 0,0,0 at the centre of the image - or something like that). Did you encounter any hints towards this when reading up and exploring the data?

@HenryCrosswell
Copy link
Author

I thought that was what line 148-153 does - downscales the image, it could also be what you suggested, ill look at the paper but im not sure i saw anything in there previously

@alessandrofelder
Copy link
Member

I thought that was what line 148-153 does - downscales the image

I see! No, this concerns the range of pixel values (ie each pixel value gets rescaled to be a number between 0 and 2^16-1), not extents of the image (ie the number of pixels in each direction), which doesn't change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants