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

REVIEW mb/fixElectrodeArray #59

Merged
merged 9 commits into from
Jan 26, 2017

Conversation

mbeyeler
Copy link
Member

@mbeyeler mbeyeler commented Jan 11, 2017

A few improvements for the ElectrodeArray class and some helper functions. Most of these are already being used in my experiment branches in one form or the other, but are now also tested and integrated. So the purpose of their existence is mostly to make my life easier. 😸

Argus I arrays now support legacy naming (L1, L2, ..., L8, M1, M2, ..., M8).

The electrode-to-retina distance depends on electrode type and electrode location. This computation has been moved to Electrode.set_height so that heights can be overridden.

Keeping track of electrode names in ElectrodeArray.names is redundant, because every Electrode has a name itself. Setting custom (or new) names would have to be done in both places, otherwise indexing might break. Therefore, I removed ElectrodeArray.names. In order to do the indexing, the loop has to be over name of every Electrode in an ElectrodeArray. In order to speed up the average lookup time, the array is traversed in random order.

The parameter tol of pulse2percept now specifies a fraction instead of an absolute value: For example, tol=0.1 will discard all pixels whose effective current is <10% of the max effective current value.

I also added a helper function that can find files in a directory whose names match a regular expression pattern.

@mbeyeler mbeyeler requested a review from arokem January 11, 2017 01:58
@coveralls
Copy link

coveralls commented Jan 11, 2017

Coverage Status

Coverage increased (+1.3%) to 87.325% when pulling 895c6f1 on mbeyeler:mb/fixElectrodeArray into a890ad0 on uwescience:master.

@coveralls
Copy link

coveralls commented Jan 11, 2017

Coverage Status

Coverage increased (+1.3%) to 87.325% when pulling 4ad631c on mbeyeler:mb/fixElectrodeArray into a890ad0 on uwescience:master.

@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage increased (+1.4%) to 87.396% when pulling ffc54c1 on mbeyeler:mb/fixElectrodeArray into a890ad0 on uwescience:master.

Copy link
Contributor

@arokem arokem left a comment

Choose a reason for hiding this comment

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

Let's change randomly so that it's specific to enumeration and rename it enumerate_randomly or enumerate_random_access to clarify what it does

# Traverse file list and look for `pattern`
filenames = []
pattern = re.compile(pattern)
for file in listdir(datapath):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change file to be something that isn't a reserved name in Python (fname would work)

@coveralls
Copy link

coveralls commented Jan 24, 2017

Coverage Status

Coverage increased (+1.4%) to 87.396% when pulling 5745b4d on mbeyeler:mb/fixElectrodeArray into a890ad0 on uwescience:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.4%) to 87.42% when pulling df9a772 on mbeyeler:mb/fixElectrodeArray into a890ad0 on uwescience:master.

@coveralls
Copy link

coveralls commented Jan 26, 2017

Coverage Status

Coverage increased (+1.4%) to 87.42% when pulling d8da8c6 on mbeyeler:mb/fixElectrodeArray into a890ad0 on uwescience:master.

@mbeyeler mbeyeler merged commit db7802b into pulse2percept:master Jan 26, 2017
@mbeyeler mbeyeler deleted the mb/fixElectrodeArray branch January 26, 2017 19:02
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.

None yet

3 participants