Conversation
|
@IgorTatarnikov has kindly volunteered to have a look at this given his experience of working on the Dorr atlas with @saarah815. Before this, could you ensure the tests pass, please? (Not sure how busy you are atm) |
|
Hey @alessandrofelder so theres a bit of a backlog of PRs. This is merging into #26 which merges into #23. I think the tests will always be too heavy to pass in an action, and the repo tests pass offline. But yeah some problems like this click import thing should definitely be fixed. Thank you |
|
eventually we will do something like this so that the tests are more light weight and runnable via gh action #28 |
|
I will run it again offline just to be sure and post the results here |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
genuinely shocked that some of the VMs did not crash and are passing 🎉 |
|
also passes on my machine offline |
OK! Sound like a plan :) I'd suggest noting on future PR descriptions with this situation that you don't expect tests to pass (because reviewers might wait for you to fix them before taking time to review otherwise). (But ideally CI tests pass before anyone is asked to review.) I'd also (possibly naively) say you can drop the CI runs for Python 3.9 and 3.10 (that were cancelled - maybe because they hit the runner VM limits somehow) - other BrainGlobe tools support Python 3.11+ only anyway. |
|
Good idea :) Done |
|
Are you ok with me merging this (and the other PR's as theyre sort of merging in a chain). |
IgorTatarnikov
left a comment
There was a problem hiding this comment.
Sorry it took so long to review!
It looks great, I just left a few comments that aren't blocking. It would be good to avoid storing the shapes of the atlases as hard coded values. It's always best to have one source of truth!
|
|
||
| VERSION = "1.0" | ||
| voxel_size_micron = 25 | ||
| perens_shape = np.array((615, 297, 455)) |
There was a problem hiding this comment.
Will perens be a common intermediary? It might be worth saving the metadata for the perens atlas as a constant somewhere to import into other files.
There was a problem hiding this comment.
i can access it through brainglobe it just then requires installing the whole atlas to get the shape. I think this will be fixed in v2 and its also nice to use our tools for things like this so ill change it.
| perens_shape = np.array((615, 297, 455)) * 25 | ||
| dorr_shape = np.array((701, 361, 538)) * 25 |
There was a problem hiding this comment.
I wonder if the shapes could be loaded from somewhere.
The shapes could be accessed via brainglobe-atlasapi, however, this would entail downloading the entire atlas which may slow things down. This will be solved with BrainGlobe Atlas V2 as you'd only fetch the metadata file and no data to return the shape.
There was a problem hiding this comment.
ah aha i see i replied to your other comment before reading this! I am ok with using the api for this :)
There was a problem hiding this comment.
one really weird issue is the dorr shape here is not actually the shape of dorr! but instead the shape of dorr if it were 25 micron voxels (to match perens). Ill jsut add scaling for this.
|
No stress! and I agree about storing atlas shapes, I'll look into a better alternative |
|
the jobs are again running out of resources but the parts of the code theyre testing havent changed since the last time it passed. I just tried to rerun some of them. This is however ready for review. Thank you |
IgorTatarnikov
left a comment
There was a problem hiding this comment.
LGTM 🎉
Thanks for addressing the comments!
|
Thanks for your review! |
* implement atlas synonyms to be more compatible with brainglobe * return atlas synonyms among near misses * Update README with atlas synonyms Added information about brainglobe atlas API names and their conversions. * precommit * precommit and add documentation about running just one test * add to ignore words list so that codespell doesnt think the method name is a typo * include demba api name in autoconvert list * update metadata and ingestion scripts to include dorr * update readme * run graph update script and update readme * run graph update script and update readme * update dorr age to p84 * update accessibility text as we are no longer using an image * update ruff formatting * run precommit * precommit * replace click with print * add mode arg to suppress future warning * ruff reformat * unittest worked but pytest failed, here we introduce a fix for this * drop workflows for 3.9 and 3.10 as they seem to run out of resources * use atlas api to access atlas shapes * remove license to make pre-commit happy * Dorr mri (#27) * include demba api name in autoconvert list * update metadata and ingestion scripts to include dorr * update readme * run graph update script and update readme * run graph update script and update readme * update dorr age to p84 * update accessibility text as we are no longer using an image * update ruff formatting * run precommit * precommit * replace click with print * add mode arg to suppress future warning * ruff reformat * unittest worked but pytest failed, here we introduce a fix for this * drop workflows for 3.9 and 3.10 as they seem to run out of resources * use atlas api to access atlas shapes * remove license to make pre-commit happy * Remove unnecessary whitespace in config.py --------- Co-authored-by: Igor Tatarnikov <61896994+IgorTatarnikov@users.noreply.github.com>
* remove testing file * auto publish to pypi on release * update yml file and python versions * remove scheduled deploy * remove twine api key as we can instead set the repo to be a trusted publisher for token less pushing * run black linter * enforce line lengths * enforce 79 char line length * exclude files that will not be included in pypi * remove line length from ruff as its failing test (its still specified elsewhere) * fix issue with test * update license file to avoid spdx warning * ruff fixes * correct line incorrectly removed from ingestion script and caught by precommit * remove unused variable caught by precommit * make ruff happy * ruff reformat * exclude utilities * precommit passed locally but failed remotely, updating reqs and license * move docstring to beginning of file * tests failing bc of triple quoted comments * ruff reformat * test off by 0.5 voxels... * rename test case * revert test value * fix empty last line * autofix files precommit * Auto detect same space atlases (#26) * implement atlas synonyms to be more compatible with brainglobe * return atlas synonyms among near misses * Update README with atlas synonyms Added information about brainglobe atlas API names and their conversions. * precommit * precommit and add documentation about running just one test * add to ignore words list so that codespell doesnt think the method name is a typo * include demba api name in autoconvert list * update metadata and ingestion scripts to include dorr * update readme * run graph update script and update readme * run graph update script and update readme * update dorr age to p84 * update accessibility text as we are no longer using an image * update ruff formatting * run precommit * precommit * replace click with print * add mode arg to suppress future warning * ruff reformat * unittest worked but pytest failed, here we introduce a fix for this * drop workflows for 3.9 and 3.10 as they seem to run out of resources * use atlas api to access atlas shapes * remove license to make pre-commit happy * Dorr mri (#27) * include demba api name in autoconvert list * update metadata and ingestion scripts to include dorr * update readme * run graph update script and update readme * run graph update script and update readme * update dorr age to p84 * update accessibility text as we are no longer using an image * update ruff formatting * run precommit * precommit * replace click with print * add mode arg to suppress future warning * ruff reformat * unittest worked but pytest failed, here we introduce a fix for this * drop workflows for 3.9 and 3.10 as they seem to run out of resources * use atlas api to access atlas shapes * remove license to make pre-commit happy * Remove unnecessary whitespace in config.py --------- Co-authored-by: Igor Tatarnikov <61896994+IgorTatarnikov@users.noreply.github.com> --------- Co-authored-by: Igor Tatarnikov <61896994+IgorTatarnikov@users.noreply.github.com>
Description
This adds the dorr mouse mri atlas to ccf translator with a deformation to perens stereotaxic mri created by @saarah815
What is this PR
References
brainglobe/brainglobe-atlasapi#657
How has this PR been tested?
ive manually checked that it produces the correct results transforming to and from perens stereotaxic mri and allen
Checklist: