Update thalamus #18
Update thalamus #18
Conversation
Changes to be committed: modified: atlas_direction_vectors/algorithms/simple_blur_gradient.py modified: atlas_direction_vectors/algorithms/utils.py
Changes to be committed: modified: atlas_direction_vectors/thalamus.py
|
Looks fine to me; I'm not able to comment on the scientific validity. |
|
Ugh, unfortunately my understanding of how to build the direction-vectors/orientations was the opposite of correct in a key assumption, so further changes are needed. I'll add those changes soon, but there's a silent external dependency here as well, where I don't want to push the direction-vectors before I've confirmed that their use in atlas-placement-hints is correct as well. This should be done by the end of May. |
Sure, no problem. If there is somewhere, documentation wise, that could have helped prevent an error, let me know. |
|
Here is the new (very small) change needed to fix the thalamus direction-vectors generation. I tried running |
I believe that it's in When I ran it, the error messages that are here: |
|
That's weird, I don't get a NaN warning when running the actual main code (not the test). Investigating |
We may be using a simplified dataset, I can't remember. |
This makes a small change to the target of the "RT-complement" part of the thalamus direction-vectors calculation. This change prevents thalamic NaN direction-vectors from being created in the test case. It should be noted that, when applied to the "real data" Atlas annotation instead of the test case, neither of these landscapes produced any NaN direction vectors, despite the previous landscape producing NaNs in the test case. The new landscape, when applied to Atlas data, seems to produce direction-vectors which are satisfactory (if interested, ask me for details). In order to pass the tests, I also had to reduce the tolerance on one of the norm tests, since in the test case, it was producing norms that further off from 1 than 1e-6. If such norms are produced in the actual data, I'm not sure what the effects will be on placement-hints, but my run of placement-hints using real data from these new direction-vectors seemd to be fine. Also, I will point out that when running the test case for the pre-2023 values of the direction vectors (including `sigma=ratio * 18.0` etc.), the vast majority of the test case voxels were not changed from their initialized values (i.e., they were not within range of the gaussian blur being applied at all). This may have led to most voxels passing this norm test as a false positive, since most voxels in the older test case were not actually modified at all, if being affected by the gaussian blur (meant to point them to RT, which they mostly were not in this case) is what likely has the capacity to change the norm from 1. In other words: the test whose tolerance I changed was being passed in the most recent test case, but I suspect that this is because the direction-vector algorithm was not changing the vast majority of voxels in the test case, when it should have been.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
=======================================
Coverage ? 90.88%
=======================================
Files ? 16
Lines ? 472
Branches ? 0
=======================================
Hits ? 429
Misses ? 43
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Whoops lol I forgot to mention that I tested this using 3.9 and it should now successfully pass |
Ok, I will try and have a look. Prod me in a couple days if I have forgotten. |
|
Thanks Mike! (my first successful PR to a BBP repo) |
This is a very small pull request for passing a "radius" argument down all the way to scipy's
gaussian_filterinatlas_direction_vectors/algorithms/utils.py, and then using it and changing some parameters used for the thalamus' direction-vectors calculation. Most of the single-line changes are just from running theisortandblackcommands as suggested in the README. Thanks!