-
Notifications
You must be signed in to change notification settings - Fork 8
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
Remove need for config file for `atlas-densities cell-densities fit-average-densities command #67
base: main
Are you sure you want to change the base?
Conversation
…verage-densities` command * The updated command doesn't require a combine_markers_ccfv2_config.yaml * Instead, the command line has: --marker=pv:868:PATH/TO/pvalb.nrrd \ --marker=sst:1001:PATH/TO/SST.nrrd \ --marker=vip:77371835:PATH/TO/VIP.nrrd \ --marker=gad67:479:PATH/TO/gad1.nrrd \ --realigned-slices=atlas_densities/app/data/markers/realigned_slices_ccfv2.json \ --cell-density-standard-deviations=atlas_densities/app/data/measurements/std_cells.json \ * As discussed in: #66
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
=======================================
Coverage ? 97.78%
=======================================
Files ? 22
Lines ? 1447
Branches ? 0
=======================================
Hits ? 1415
Misses ? 32
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I think more changes are necessary to make the code really generic. |
That can be done; I think that would obviate the need for the
Which code are you talking about? |
Maybe a folder path could be given? With expected filenames in it?
The code Dimitri shared on the atlas channel. See with @drodarie, he created such a convertion. |
No, we should always make things explicit, so it's clear what is required.
I assume you mean this code: https://bluebrainproject.slack.com/archives/C016G4U2C8H/p1708092444378119 |
I created a script to merge all the json results from Deep Atlas into a single json file that could be consumed by the atlas-densities pipeline. I agree with @Sebastien-PILUSO that having one json per experiment instead of adding an extra step is better. However, IMO, I think this idea of removing configuration file goes in the wrong direction with respect to the future of the pipeline. Let's imagine you decide to change the composition rule for inhibitory neuron: now it is GAD2 = PV + SST + GENE4 or worse if you want to use T-types. Apart from that, I do not mind too much the changes proposed here. This is just a change of interface. |
The same information is conveyed in this, it just means that one doesn't need to change a file to make changes to which files are used; and it's one less file to keep track of. If there are big changes, like you said, we'll have to refactor everything regardless. |
So how about this correction of the code for adding the input json files as input? |
I have been away on vacation, and am currently catching up. To confir, the final proposal for the command line is:
|
--marker=pv:868:PATH/TO/pvalb.nrrd
--marker=sst:1001:PATH/TO/SST.nrrd
--marker=vip:77371835:PATH/TO/VIP.nrrd
--marker=gad67:479:PATH/TO/gad1.nrrd
--realigned-slices=atlas_densities/app/data/markers/realigned_slices_ccfv2.json
--cell-density-standard-deviations=atlas_densities/app/data/measurements/std_cells.json