-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fixes to Perens and Kim mouse atlases #271
Conversation
Errors to be solved:
perens |
Atlas generation on HPC:
|
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 am confused about the argument parser introduced here.
Otherwise looks great :)
parser = argparse.ArgumentParser( | ||
description="Create an atlas with a specified resolution." | ||
) | ||
parser.add_argument( | ||
"--resolution", | ||
type=int, | ||
default=10, | ||
help="Resolution in microns (10, 25, 50, 100)", | ||
) | ||
args = parser.parse_args() |
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.
Why are we adding an argument parser here?
I think it should be a loop over the resolutions we'd like to provide?
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.
Yeah I think the atlas generation scripts are meant to (?) loop over resolutions.
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 didn't do it. Only one atlas was generated when I submitted a job on the HPC and I didn't want to change the resolution value in the code manually, push the new version and submit a new job for each resolution. I might be missing something.
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 didn't want to change the resolution value in the code manually, push the new version and submit a new job for each resolution.
I agree this is a worthy goal - my point is that I think this should not be achieved by a command line argument, but rather with a for loop in the code (so the script generates all four atlases in the same run)
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 thought this was already in the code, but it's what the main script (see #273) coordinates.
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 see what you mean now. Will I still have the option to easily specify which atlas I want to generate? For example, if I only need the 100um one, I wouldn't want to wait for the script to generate all the others.
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 don't think you will, no.
If you'd like that flexibility, you probably want to clone the repository on the HPC, and edit the script via the command line?
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.
Having thought about this, I think it's best to:
- Stick with what you have now
- Later on, remove it and sort out the main script (unless anyone has the capacity to address Decide what to do with
atlas_generation/main_script
#273 now?
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 think we commented at the same time @alessandrofelder. I think we ideally shouldn't add the parser to the script, but getting fixed versions of the atlases out should be a priority.
Passed the validation functions (without the additional_references functions, see #275):
kim_mouse_10um is still being regenerated |
kim_mouse_10um passed the validation functions |
Does this also close #247? |
Yes, it does |
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.
- Quick check that locally the issue is gone was successful ✅
- Opened [Feature] flexibly generate atlases with different resolutions #280 to think further about what the packager's interface should be for multi-res atlases
Description
This PR fixes
IndexError: tuple index out of range
ofperens_lsfm_mouse.py
(see: #249) and the orientation and scaling issue of perens_lsfm_moue and kim_mouse atlases mentioned in #265What is this PR
What does this PR do?
fixes #265
References
#265
How has this PR been tested?
tested on HPC and locally
Is this a breaking change?
no
Does this PR require an update to the documentation?
Yes, when new versions of the atlases are published
Checklist: