search

IHEC / ihec-ecosystems

This repo is for code and documentation associated with the ihec-ecosystems working group
Apache License 2.0
5 stars 6 forks source link

Fix not allowing histone NA for ChIP-Seq input #116

Closed sitag closed 3 years ago

sitag commented 3 years ago

Fix the experiment semantic check bug with size one lists 1 Fix not allowing histone NA for ChIP-Seq input

dzerbino commented 3 years ago

Dear @sitag ,

Good point, this would create problems.

I am wondering whether it could be done in a more direct way, because classifying ChIP-Seq inputs as chromatin marks (as opposed to TF) is arbitrary. Note that the experiment_type is one of input, histone or TF.

I think it would be cleaner to make the required attributes optional if experiment_type == "ChIP-Seq Input", rather than adding NA as a histone mark.

Cheers,

Daniel

sitag commented 3 years ago

@dzerbino The target_tf right now is required. It does not have a controlled vocabulary so supports NA. To treat both equally we should support NA for histone too. I can update the semantic check to test for both of them being NA for ChIP Input,

dzerbino commented 3 years ago

Hello @sitag ,

Would it not be simpler to state that if the experiment type is Input, then target_tf and target_histone should not be provided? I'm not a big fan of having a hard-coded 'NA' value, when it would be cleaner and simpler to not define these attributes at all.

Cheers,

Daniel

sitag commented 3 years ago

@dzerbino That's how ChIP-Input was setup earlier. Then @dbujold requsted that ChIP-Input and ChIP be coalesced with NA for not used items. Now spec requires them all https://github.com/IHEC/ihec-ecosystems/blob/master/schemas/json/2.0/experiment.json#L121-L147 Making anything optional breaks that scheme.

dzerbino commented 3 years ago

OK, fair enough.