BlueBrain / BluePyMM

Blue Brain Python Cell Model Management
https://bluepymm.readthedocs.io/en/latest/
Other
12 stars 9 forks source link

tsv output should not contain empty value for threshold and holding current #222

Open jdcourcol opened 3 years ago

jdcourcol commented 3 years ago

it happens that the mecombo_emodel.tsv contains missing entries for threshold or holding current, leading to rows with 7 entries instead of 8 for instance. This is incompatible with what the simulator expects (it expects 8 values to be provided).

Probably it relates to that line: https://github.com/BlueBrain/BluePyMM/blob/1f87b5125d178a0a6d65672e612fefbca38ae65d/bluepymm/select_combos/megate_output.py#L94

We may consider to discard the cells from the output for instance.

arnaudon commented 3 years ago

Did you get this warning message

https://github.com/BlueBrain/BluePyMM/blob/1f87b5125d178a0a6d65672e612fefbca38ae65d/bluepymm/select_combos/megate_output.py#L100

in your logs?

@wvangeit , shall we:

  1. keep as it is
  2. keep the warning and discard bad rows to get a clean output (in all files, not just extneurondb)
  3. make MM crash as it is in the commented code?

I would be in favor of option 2, as jean-denis suggested.

wvangeit commented 3 years ago

As this shows, it looks like warnings should not be used, because nobody reads warnings. Maybe the safest way is to make it crash by default, and have a flags like remove_rows_with_empty_threshold / remove_rows_with_empty_holding. What do you think?

The only thing is, does it add an empty column, or no column at all? Maybe it would already be better to at least make sure an empty column is added?

arnaudon commented 3 years ago

Yes, this seems a good solution. Not sure which column you mean, w'll keep the same columns, but remove some rows, right?

jdcourcol commented 3 years ago

@arnaudon did you take a decision ?

arnaudon commented 3 years ago

I'll implement what werner proposed!

arnaudon commented 3 years ago

PR here: https://github.com/BlueBrain/BluePyMM/pull/225