fix: keep group index and result position apart in grouped estimation (#318) - #322
Open
MohammadYusif wants to merge 1 commit into
Open
fix: keep group index and result position apart in grouped estimation (#318)#322MohammadYusif wants to merge 1 commit into
MohammadYusif wants to merge 1 commit into
Conversation
…GWeindel#318) _estim_probs_groups collected the per-group results in a list built by iterating over np.unique(groups), but then indexed that list with the group itself. Both only coincide when every declared group occurs in the data: as soon as a group declared in the channel/time maps has no trial left, the positional list and the group index disagree, event probabilities are written to the wrong trials and the loop ends on an IndexError. The per-group log-likelihoods had the same problem. They were returned as a compacted array over the groups present in the data, while group_labels covers every declared group, so group_lkh (and the EM traces built from it) silently stopped lining up with the groups they describe. Keep the position in the result list and the group index separate, and size the likelihood array on the declared groups, leaving nan for a group without trials.
Owner
|
Thank you for your pull request, could we maybe have a quick online meeting regarding your suggestion? You can send me an email at gabriel.weindel@unil.ch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EventModel._estim_probs_groupscollects the per-group results in a list built positionallyover
np.unique(groups), but then indexes that list with the group itself. The two onlycoincide when every declared group occurs in the data — as soon as a group declared in
channel_map/time_maphas no trial left (a level absent after trial rejection, aparticipant subset missing a condition), event probabilities are written onto the wrong
trials and the loop ends on
IndexError: list index out of range.returned as a compacted array over only the groups present in the data, while
group_labelscovers every declared group, sogroup_lkh— and thetraces_group/xrtracesbuilt from it inEM— silently stopped lining up with the groups they describe.likelihood array on the declared groups, leaving
nanfor a group without trials.As mentioned in the issue, the alternative would be to reject a declared group without
trials outright. I went with indexing by position because a model fitted on the full data
and then transformed on a subset that happens to miss a condition is a legitimate case, but
happy to switch if you'd rather not allow empty groups at all.
Changes
hmp/models/event.py: in_estim_probs_groups, iterate withenumerate(data_groups)so the result position and the group index stay apart;pre-size
likelihoodon the declared groups (np.zeros(self.channel_map.shape[0]) * np.nan)and fill it by group, so
group_lkhstays aligned withgroup_labels; sum it withnp.nansum, which is unchanged whenever every group is present.tests/test_fixed.py:test_grouping_absent_groupfits the same trials twice with fullyindependent per-group parameters — once coded
0/2with the declared group1leftwithout trials, once coded
0/1with only the two occurring groups declared — andasserts the likelihood and both parameter arrays match, that the group likelihoods are
reported for every declared group, and that the empty one is
nan.Fixes #318