-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cjg/read inputs #23
Cjg/read inputs #23
Conversation
…ands use RLEVec to encode sequence data while input commands use plain vectors.
…dSets for constant per-rig lists of channels. Add an automatic load_signals function that tries to load all relevant input and output signals for the experiment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I confess I didn't look over the parse*
stuff, but I left a few comments elsewhere. I should acknowledge that I haven't looked over the source as a whole in a while, so some of these remarks could be off-target.
src/convenience.jl
Outdated
findname(coms::AbstractArray{ImagineCommand,1}, nm::String) = findfirst(x->name(x) == nm, coms) | ||
function getname(coms::AbstractArray{ImagineCommand,1}, nm::String) | ||
findname(coms::AbstractArray{ImagineCommand,1}, nm::AbstractString) = findfirst(x->name(x) == nm, coms) | ||
function getname(coms::AbstractArray{ImagineCommand,1}, nm::AbstractString) | ||
namei = findname(coms, nm) | ||
if namei == 0 | ||
error("Name not found") | ||
else | ||
coms[findname(coms, nm)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use namei
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yes :-)
src/convenience.jl
Outdated
findname(coms::AbstractArray{ImagineCommand,1}, nm::String) = findfirst(x->name(x) == nm, coms) | ||
function getname(coms::AbstractArray{ImagineCommand,1}, nm::String) | ||
findname(coms::AbstractArray{ImagineCommand,1}, nm::AbstractString) = findfirst(x->name(x) == nm, coms) | ||
function getname(coms::AbstractArray{ImagineCommand,1}, nm::AbstractString) | ||
namei = findname(coms, nm) | ||
if namei == 0 | ||
error("Name not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Name $fm not found"?
src/hardware_constants.jl
Outdated
|
||
#Lists of analog output channels | ||
const ocpi1_aochans = map(x->"AO$(x)", 0:1) | ||
const ocpi2_aochans = map(x->"AO$(x)", 0:3) | ||
const AO_CHANS= Dict("ocpi-1" => ocpi1_aochans, | ||
"ocpi-2" => ocpi2_aochans) | ||
const ocpi_lsk_aochans = map(x->"AO$(x)", [0;2;3]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have rigs/ocpi1.jl
, rigs/ocpi2.jl
, rigs/lsk.jl
files? If we open this up to the rest of the world, we won't want to have to add ocpi_alabama_state_university_deptbio_core1_aochans
to this file. 😄
Might want instructions on writing a rigs
file. Also worth considering whether rigs
should live in this source tree or whether there should be some standard system path. Cross-platform issues make the latter a bit complicated.
Alternatively, is there some rig config file that both Imagine and this repo can share?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have rigs/ocpi1.jl, rigs/ocpi2.jl, rigs/lsk.jl files?
I think this makes sense. And I think that organization would favor creating some abstraction for what a rig is. I've considered creating some kind of Rig
or RigConfiguration
type but didn't do it. I think it would be worthwhile. For now I will just break this into separate files and consider that abstraction in a later PR.
Alternatively, is there some rig config file that both Imagine and this repo can share?
This would be my favorite solution, though it isn't the easiest one. We do have dedicated javascript rig configuration files for Imagine, but last time I checked they only contained a fraction of the information that I include in hardware_constants.jl
. @kdw503 knows better the current contents of these files and how much work it might take to include all of the info that I'm keeping in this repository. @kdw503 what do you think of the idea of sharing configuration files? If we decide to do it, then maybe it makes sense to switch these files to JSON to match our command files? Whatever format you choose I'm willing to make the necessary changes to this package if you handle the changes in Imagine.
How about I split this into separate files for now and get this PR merged, then we can work on a common rig file separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay ecc51f9 eliminates hardware_constants.jl
in favor of separate files for each rig. For now the rig directory is still in this source tree. I think it's worth abstracting channels, rigs, and probably also cameras in order to make this more elegant. But for now it's working, and new rigs can be added by mimicking the format of the individual rig files.
…ons, some convenience functions, rename getmonitor_name
…older. Also clarify a find / get function.
Tests were failing on 0.5 and I decided it wasn't worth fixing. Seems like we are all moving to 0.6. |
Jameson's fix of issue 265 coupled with Revise.jl has so completely changed my workflow that I have difficulty imagining using anything older than 0.6 now. |
Great! I'll go ahead and merge this then. Opening another issue for sharing rig config files with Imagine. |
Support reading of analog input ".ai" and digital input ".di" files recorded during an Imagine session. Some changes were required to the ".imagine" format, so this will require timholy/ImagineFormat.jl#11 to work.