BlueBrain / atlas-densities

Tools to compute densities in the context of brain atlases.
Apache License 2.0
3 stars 8 forks source link

Stop using as many config files #38

Closed edasubert closed 1 year ago

edasubert commented 1 year ago

new command line: OUTPUT=output/create-from-probability-map DATA=../data atlas-densities \ mtype-densities -vv create-from-probability-map \ --hierarchy-path=$DATA/1.json \ --annotation-path=$DATA/ccfv2/annotation_25.nrrd \ --probability-map probability_map.csv \ --marker gad67 $DATA/create-from-probability-map/gad67.nrrd \ --marker sst $DATA/create-from-probability-map/sst.nrrd \ --marker vip $DATA/create-from-probability-map/vip.nrrd \ --marker approx_lamp5 $DATA/create-from-probability-map/approx_lamp5.nrrd \ --marker pv $DATA/create-from-probability-map/pv.nrrd \ --output-dir=$OUTPUT

Thus --metadata-path and --mtypes-config-path aren't used any more

codecov-commenter commented 1 year ago

Codecov Report

Merging #38 (8f4ad1d) into NSETM-2196-update-create-from-probability-map-probability-map-faster (b701fb7) will decrease coverage by 0.02%. The diff coverage is 100.00%.

@@                                           Coverage Diff                                            @@
##           NSETM-2196-update-create-from-probability-map-probability-map-faster      #38      +/-   ##
========================================================================================================
- Coverage                                                                 97.78%   97.77%   -0.02%     
========================================================================================================
  Files                                                                        22       22              
  Lines                                                                      1357     1348       -9     
========================================================================================================
- Hits                                                                       1327     1318       -9     
  Misses                                                                       30       30              
Flag Coverage Δ
pytest 97.77% <100.00%> (-0.02%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ities/densities/mtype_densities_from_map/create.py 100.00% <100.00%> (ø)
...sities/densities/mtype_densities_from_map/utils.py 100.00% <100.00%> (ø)
lecriste commented 1 year ago

@edasubert, @mgeplf, I noticed that this PR makes the ME-type densities dependent on the markers and no longer on the inhibitory/excitatory neuron densities, namely {gad67+, pv+, sst+, vip+}_density.nrrd listed in the yaml config. Is it intended?

mgeplf commented 1 year ago

Correct, the idea is to remove the need for having that config file, since if one is programmatically launching the code, it's easier to modify a series of command line arguments than changing a config.

lecriste commented 1 year ago

Sure but I mean that the CL arguments being used by this PR are pointing to a set of files (markers) different from the set of files in the previous config (inhibitory/excitatory neuron densities). Is this files change intended?

edasubert commented 1 year ago

I am not sure what you mean? the paths in examples are sort of arbitrary since the files don't come in the repository, or am I missing something?

lecriste commented 1 year ago

The files don't come in the repository but the description of the yaml config reads

The probability map, together with the volumetric densities of several molecular types (e.g., the types designating the neurons which react to PV, SST, VIP and GAD67)

and the Note at the end says

the volumetric density nrrd file of a molecular type (e.g., pv.nrrd) should not be confused with the AIBS gene expression volume with the same name which has been used to generate the density field.

mgeplf commented 1 year ago

The files are the same though; whether they were referenced the old way through the config file, or whether they are passed through the command line, their contents are the same.

Updating the README that comes w/ the config might be useful, but we'd have to look at where else it is being used.

lecriste commented 1 year ago

The files are the same though; whether they were referenced the old way through the config file, or whether they are passed through the command line, their contents are the same.

The files passed through the command line arguments introduced by this PR are the markers registered in Nexus (plus the "approx_lamp5"), the files referenced through the old config are the output of the inhibitory/excitatory neuron densities step. Is their content the same?

mgeplf commented 1 year ago

It looks like it's a terminology issue; the diff has this: https://github.com/BlueBrain/atlas-densities/pull/38/files#diff-9ad6b3bab65838e09f2b4c1dd28e4b51fd8fde67cbee1584b92af97a0a9d5b16R255

So basically what was in the molecularTypeDensityPaths should be passed into the marker arguments.

lecriste commented 1 year ago

Exactly. As Eduard said, the paths in the old config example are sort of arbitrary since the files don't come in the repository and, based on the config description, the pipeline is setting them to the output of the inhibitory/excitatory neuron densities step.

So basically what was in the molecularTypeDensityPaths should be passed into the marker arguments.

I would then rename the "marker" argument to something less ambiguous.

mgeplf commented 1 year ago

I would then rename the "marker" argument to something less ambiguous.

Ok, we can do that; what do you think would be a good name? --marker-neuron-density-estimates?

lecriste commented 1 year ago

Yes, or just --neuron-density-estimates.

mgeplf commented 1 year ago

Yes, or just --neuron-density-estimates

Their not just neuron estimates, though, since they are keyed on marker names, no?

lecriste commented 1 year ago

Yes but I don't see marker mentioned in the step producing those: https://github.com/BlueBrain/atlas-densities/blob/16981b5d244d9cdde82ed6b098564f8296c2dbfa/README.rst#bbp-cell-atlas-version-2

Anyways, both versions are fine with me.