Skip to content
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

Merged
merged 11 commits into from
Jul 21, 2017
Merged

Cjg/read inputs #23

merged 11 commits into from
Jul 21, 2017

Conversation

Cody-G
Copy link
Contributor

@Cody-G Cody-G commented Jul 11, 2017

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.

@Cody-G Cody-G force-pushed the cjg/read_inputs branch from 138350d to 54c5601 Compare July 11, 2017 15:18
Cody-G added 4 commits July 11, 2017 10:21
…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.
@Cody-G Cody-G force-pushed the cjg/read_inputs branch from 54c5601 to 31f6d15 Compare July 11, 2017 15:21
Copy link
Member

@timholy timholy left a 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.

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)]
Copy link
Member

Choose a reason for hiding this comment

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

Just use namei?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yes :-)

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")
Copy link
Member

Choose a reason for hiding this comment

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

"Name $fm not found"?


#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])
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

@Cody-G Cody-G force-pushed the cjg/read_inputs branch from ecc51f9 to 4028b63 Compare July 21, 2017 17:08
@Cody-G
Copy link
Contributor Author

Cody-G commented Jul 21, 2017

Tests were failing on 0.5 and I decided it wasn't worth fixing. Seems like we are all moving to 0.6.

@timholy
Copy link
Member

timholy commented Jul 21, 2017

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.

@Cody-G
Copy link
Contributor Author

Cody-G commented Jul 21, 2017

Great! I'll go ahead and merge this then. Opening another issue for sharing rig config files with Imagine.

@Cody-G Cody-G merged commit f6723d6 into master Jul 21, 2017
@Cody-G Cody-G deleted the cjg/read_inputs branch August 11, 2017 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants