Conversation
|
Since this PR now includes the reformatting required by the bg tests its quite a few LOC, but ofc thats just formatting changes. |
|
the ebrains storage is currently offline which this package relies on which is why the tests are failing. I'll mark it ready for review once this is back online. |
|
this is ready for review. I believe the remote tests will fail as they are too heavy duty for a github action however I have run them locally and they are passing |
IgorTatarnikov
left a comment
IgorTatarnikov
left a comment
There was a problem hiding this comment.
Just a few comments regarding consistency/standardisation throughout. Feel free to leave those as separate issues that can be fixed in future PRs.
I would also suggest changing the imports for brainglobe-ccf-translator to be absolute as opposed to relative throughout. At this point it's used interchangeably and standardising it would be nice!
| - os: macos-latest | ||
| python-version: "3.11" | ||
| - os: windows-latest | ||
| python-version: "3.11" |
There was a problem hiding this comment.
We typically have these set to 3.13 (or the latest compatible version of Python being tested)
| segmentation_file=dictionary["segmentation_file"], | ||
| ) | ||
| except: | ||
| except Exception: |
There was a problem hiding this comment.
Catching a broad exception is best avoided. Could this be made more specific?
|
|
||
| ccft_vol.transform(target_age, "perens_multimodal_lsfm") | ||
| ccft_vol.save(rf"../demo_data/perens_lsfm_from_princeton.nii.gz") | ||
| ccft_vol.save(r"../demo_data/perens_lsfm_from_princeton.nii.gz") |
There was a problem hiding this comment.
The use of raw strings in ccft.Volume.save is inconsistent between tutorials/transform_brainglobe_volume.py and tutorials/view_allen_connectivity.py. I think the r can be removed here and on line 26.
| save_path = os.path.expanduser(f"~/.brainglobe/deformation_fields/{space_name}") | ||
| save_path = os.path.expanduser( | ||
| f"~/.brainglobe/deformation_fields/{space_name}" | ||
| ) |
There was a problem hiding this comment.
Could all path related functions be standardised to use the same method, (e.g. all paths are Path objects)
There was a problem hiding this comment.
Is this file necessary? The instructions can be found https://brainglobe.info/community/developers/new_releases.html
| name = "brainglobe_ccf_translator" | ||
| description = "a package to translate data between common coordinate frameworks" | ||
| readme = "README.md" | ||
| license = { file = "LICENSE" } |
There was a problem hiding this comment.
I think this is okay to keep as license = {file = "LICENSE"} or as license = {text = "MIT"} based on the NIU cookiecutter
| "Development Status :: 5 - Production/Stable", | ||
| "Intended Audience :: Science/Research", | ||
| "Topic :: Scientific/Engineering :: Bio-Informatics", | ||
| "License :: OSI Approved :: MIT License", |
There was a problem hiding this comment.
This could stay as well based on the NIU cookiecutter
| 3.9: py39 | ||
| 3.10: py310 | ||
| 3.11: py311 | ||
| 3.12: py312 | ||
| 3.13: py313 |
There was a problem hiding this comment.
I think py39 and py310 can be dropped here
| #"C90", # McCabe complexity | ||
| ] | ||
| lint.ignore = [ | ||
| "E501", # line too long - let Black handle formatting |
There was a problem hiding this comment.
Is there a specific reason to delegate to black?
* 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>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
======================================
Coverage ? 0.00%
======================================
Files ? 13
Lines ? 660
Branches ? 0
======================================
Hits ? 0
Misses ? 660
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
After addressing the issues in the previous PR we are now using the same action as other brainglobe projects. The main change is I have set this repo to be a trusted publisher so we no longer require the twine api key from pypi.