BlueBrain / atlas-densities

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

L3_TPC:B type label should be replaced with L3_TPC:C #35

Closed crisely09 closed 9 months ago

crisely09 commented 1 year ago

Some time ago they decided that this was an incorrect label for these MTypes, so everything that uses L3_TPC:B as MType should be changed to L3_TPC:C

Related to this Ticket.

mgeplf commented 1 year ago

@lecriste Isn't this something that can be controlled by changing the configuration input files? If we're not making that configuration accessible, that's something we have to change.

lecriste commented 1 year ago

@mgeplf, which configuration files? I can see both L3_TPC:B and L3_TPC:C present in several files in this repo.

mgeplf commented 1 year ago

Which step is creating the the L3_TPC densities? I'd suspect it's the excitatory-split stage, so whatever is being passed in to --cortex-all-to-exc-mtypes.

lecriste commented 1 year ago

It's this file. What about the other occurrences of L3_TPC:B in the repo?

mgeplf commented 1 year ago

I think they are there as example configs, and as part of testing. Thus, I think they can lag what the 'state of the art' configs are.

dkeller9 commented 1 year ago

All L3_TPC:B should be changed to L3_TPC:C

dkeller9 commented 1 year ago

I changed the csv file in the "Csv update" pull request.

mgeplf commented 1 year ago

We will discuss this in the atlas meeting.

dkeller9 commented 1 year ago

@mgeplf i am getting a lint error in the "csv update" PR. Do you know why this might be occurring? https://github.com/BlueBrain/atlas-densities/actions/runs/5486872473/jobs/9997539596

mgeplf commented 1 year ago

I will look into it.

mgeplf commented 1 year ago

This is an issue with upstream dependencies, which will likely be solved shortly: https://github.com/pallets/click/issues/2558

However, this should stop us from testing the changes, to make sure the correct outputs are created. Can @dkeller9 and @lecriste verify this? In addition, I don't think it's practical that we are making configuration changes in a code repository that requires a release to get into production. I think that configuration should be tracked, versioned and validated separately.

dkeller9 commented 1 year ago

We should just verify that the [L3_TPC:C] nrrd after the change is the same as [L3_TPC:B] before the change. @lecriste can you perform this check?

lecriste commented 12 months ago

@dkeller9, check performed. Out of curiosity, in which way such a label change could affect the file content?

dkeller9 commented 12 months ago

It should not affect the file content, but it is good to verify that this is the case. If the label change had thrown a hidden error and the file was not created or nans, that would have been a problem.

lecriste commented 9 months ago

The new METype density is in Nexus prod: https://bbp.epfl.ch/nexus/v1/resources/bbp/atlas/_/https:%2F%2Fbbp.epfl.ch%2Fdata%2Fbbp%2Fatlas%2Fbb1364df-9b78-4dbb-bacf-ed6927f747e3

and in the CellComposition.

crisely09 commented 9 months ago

Nice, thanks @lecriste