Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allen bbp ccfv3a entire mouse brain, improve mesh utils, and add metadata filtering to wrapup #500
base: main
Are you sure you want to change the base?
Allen bbp ccfv3a entire mouse brain, improve mesh utils, and add metadata filtering to wrapup #500
Changes from 6 commits
49eeabe
5261b36
66c177d
1aac740
debb940
d9124c6
d4e1312
c98e96b
6ca6590
42d4514
d8fb861
3b637d7
0ec24a3
6994679
cae7982
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strong opinion, but maybe a worry would be that using a dataclass (while nice syntactic sugar for more experienced developers) introduces a small barrier for novice programmers to contribute atlases in the future (because they need to understand what
e.g. METADATA.name
refers to)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting point, I don't think its so difficult for novices but its hard to say. I think the main point of this change is to organise all the metadata in one place with detailed descriptions of what each field requires and what type of data it expects. I think doing this with constants and comments in a template file is less than ideal perhaps but I'm open to opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, it's a trade-off. Not sure - will bring up at dev meeting tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote against dataclasses. Atlas contributors tend to be the least experienced contributors we have (some have no Python experience). Anything we can do to simplify this process is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point! should I remove it from this script or is it ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove it for consistency across packaging scripts?
Check warning on line 23 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L23
Check warning on line 28 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L28
Check warning on line 250 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L250
Check warning on line 277 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L276-L277
Check warning on line 280 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L279-L280
Check warning on line 283 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L282-L283
Check warning on line 287 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L285-L287
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether these should be exposed as parameters, with default values, so we can re-use this function across more packaging scripts (as and when we need to, moving to version 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I exposed these parameters. I havent tested it yet as im currently generating demba files (and will be for a while!). There is the option to expose all of the parameters of extract mesh from mask but I'm not to familiar with that function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it runs :)
Check warning on line 289 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L289
Check warning on line 294 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L294
Check warning on line 316 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L307-L316
Check warning on line 319 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L318-L319
Check warning on line 321 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L321
Check warning on line 327 in brainglobe_atlasapi/atlas_generation/mesh_utils.py
brainglobe_atlasapi/atlas_generation/mesh_utils.py#L327