Skip to content
This repository has been archived by the owner on Sep 7, 2018. It is now read-only.

RFC: add channel_map and version to codebook #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ambrosejcarr
Copy link
Member

@ambrosejcarr ambrosejcarr commented Aug 18, 2018

This PR is a first attempt at adding a version and code_map to the codebook. Note that the diff is much smaller than git makes it appear -- to add properties to the codebook it needed to be converted from an array to an object, which caused it to get shifted up a level.

See examples for the proposed codebook structure.

@berl does this address the concern about being able to detect the channels?

Addresses #3, #10

@ambrosejcarr ambrosejcarr force-pushed the ajc-codebook-with-version-channel-map branch from 2f1703d to 79f3615 Compare August 18, 2018 23:50
Copy link

@berl berl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about "codebook" instead of "codes"? I like the list of codewords being the codebook, but either way is fine with me

@ambrosejcarr
Copy link
Member Author

@berl I like that suggestion. Can you think of a more general term for the codebook.json file now that it also has a channel map? Or is codebook still optimal?

}
],
"channel_map": [
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not do a map of channel name to index val? at least you gain channel name uniqueness as a freebie.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also see having it as an array. cF. https://github.com/IDR/idr-metadata/blob/403b0817c1c57dd6b509cfb0d0d41bd35ec1b938/idr0006-fong-nuclearbodies/screenA/idr0006-screenA-renderdef.yml

1:
  label: DAPI
  min: 0
  max: 1000
  color: "0000FF"
2:
  label: TRITC
  min: 50,
  max: 800

since an ordering is guaranteed.

Copy link

@dganguli dganguli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @berl @ambrosejcarr is channel_map.json required for the spaceTx pipeline? does it make more sense for this to live in experiment.json?

@berl
Copy link

berl commented Aug 22, 2018

channel_map.json is not strictly required for the pipeline, but it would make some things a little easier to read. E.g. asking to filter images differently based on channel, like [ch in channel_map if ch["channel"] in "488"] differently than images with [ch in channel_map if ch["channel"] in "647"]. or maybe that's just ugly anyway

@ambrosejcarr
Copy link
Member Author

Thanks all for comments. I'm going to wait to land this until we construct the dependency tree between sptx-format, slicedimage, and starfish.

@ttung
Copy link

ttung commented Aug 27, 2018

Thanks all for comments. I'm going to wait to land this until we construct the dependency tree between sptx-format, slicedimage, and starfish.

landing post-merge might be a serious PITA.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants