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

make activation consistent #87

Merged
merged 12 commits into from
Apr 19, 2017
Merged

make activation consistent #87

merged 12 commits into from
Apr 19, 2017

Conversation

Trybnetic
Copy link
Member

activation was not forced to take the correct weight dimensions.

This PR fixes this and makes the dimensions of weights consistent in pyndl

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 00cba28 on activation-checks into 4f43ded on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 00cba28 on activation-checks into 4f43ded on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 00cba28 on activation-checks into 4f43ded on master.

@coveralls
Copy link

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 17a2267 on activation-checks into 4f43ded on master.

@Trybnetic
Copy link
Member Author

Besides randomly throwing errors, pylint always stops at some point without raising an error or something like that..

@derNarr @dekuenstle have you any idea why this happens?

@Trybnetic
Copy link
Member Author

Seems like the random errors came from not giving fixed dimensions.

dim=('outcomes', 'cues') should have fixed it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling aa613db on activation-checks into 4f43ded on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling aa613db on activation-checks into 4f43ded on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling aa613db on activation-checks into 4f43ded on master.

@@ -22,7 +22,7 @@ def activation(event_list, weights, number_of_threads=1, remove_duplicates=None,
event_list : generator or str
generates cues, outcomes pairs or the path to the event file
weights : xarray.DataArray or dict[dict[float]]
the xarray.DataArray needs to have the dimensions 'cues' and 'outcomes'
the xarray.DataArray needs to have the dimensions 'outcomes' and 'cues'
Copy link
Member

Choose a reason for hiding this comment

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

pyndl.ndl.ndl has also order 'cues','outcomes'. We have to keep this consistent. Why did you change the order?

Copy link
Member Author

Choose a reason for hiding this comment

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

pyndl.ndl.ndl returns dimensions ('outcomes', 'cues'), so now activation is consistent with this.

But the doc string in ndl.py

"""
    Returns
    -------
    weights : xarray.DataArray
        with dimensions 'cues' and 'outcomes'. You can lookup the weights
        between a cue and an outcome with ``weights.loc[{'outcomes': outcome,
        'cues': cue}]`` or ``weights.loc[outcome].loc[cue]``.
"""

should be

"""
    Returns
    -------
    weights : xarray.DataArray
        with dimensions 'outcomes' and 'cues'. You can lookup the weights
        between a cue and an outcome with ``weights.loc[{'outcomes': outcome,
        'cues': cue}]`` or ``weights.loc[outcome].loc[cue]``.
"""

@coveralls
Copy link

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 5684428 on activation-checks into 4f43ded on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 5684428 on activation-checks into 4f43ded on master.

@coveralls
Copy link

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 46923b8 on activation-checks into 4f43ded on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 47862f5 on activation-checks into 97efb77 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 47862f5 on activation-checks into 97efb77 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 47862f5 on activation-checks into 97efb77 on master.

Harmonizes return value dimension order with activation dict of dicts and weight
matrices.
So it's always weights[outcome][cue] and activations[outcome][event]
@dekuenstle
Copy link
Member

@Trybnetic i added also some changes, so please check them before merging.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 98f0301 on activation-checks into 97efb77 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 98f0301 on activation-checks into 97efb77 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling 98f0301 on activation-checks into 97efb77 on master.

@Trybnetic
Copy link
Member Author

@dekuenstle I have discussed it with @derNarr. He also argues to use outcomes as the first dimension of the activation matrix.

I just did two small changes in the tests so we now use directly the matrix with the right dimensions instead of transposing the old ones

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling acd6b93 on activation-checks into 97efb77 on master.

1 similar comment
@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.3%) to 59.341% when pulling acd6b93 on activation-checks into 97efb77 on master.

@Trybnetic
Copy link
Member Author

Before merging we should check that the examples are still working! Due to the major changes in this PR I expect that they will not work anymore

@Trybnetic
Copy link
Member Author

@derNarr @dekuenstle @sumny I could not find an example of activation in the docs, is this right? As activation is a quite useful function we should consider adding an example if there isn't one yet. But I'm not sure whether we should do this in this PR or in an extra one in which we enhance the docs in general.

What do you think?

@dekuenstle
Copy link
Member

@Trybnetic Better an extra issure + PR for extending docs with activation estimation.

@Trybnetic
Copy link
Member Author

@dekuenstle I agree with you. I've made todo note in #5 as this issue is mostly about documentation and examples.

@Trybnetic Trybnetic merged commit cffe67f into master Apr 19, 2017
@Trybnetic Trybnetic deleted the activation-checks branch April 19, 2017 13:24
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.

3 participants