search

mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.68k stars 1.31k forks source link

persyst annotations error #8292

Open infinitejest opened 4 years ago

infinitejest commented 4 years ago

Describe the bug

The persyst raw file function is able to open the persyst files and annotations with no issues. The issue I am having. For my particular use case we need to get all annotations regardless if they are repeated. When some of the EEG systems generate annoations they use xlevents or xlspikes and sometimes they can be multiple of these in the files. however the parser is only taking unique annotations. So if a human annotator or machine annotator uses same annotations then the parser is not picking up all of the annotations but rather unique names or the first one with that name.

Steps to reproduce

Open any lay/dat files that have annotations with different times and same name/description

Expected results

The expected behavior is that the parser should be able to pick up all annotations even if they have the same name.

Actual results

only picks up the first annotation with that name.

Additional information

If I have time I can try to submit a suggestion on how to fix it.

I think the issue is that you are using the comment texts as keys for your dictionary of annotations. So when your script finds a new comment with the same text is overwriting the previous one. Perhaps one suggestions to append the values to your keys as a list if the keys are already in the dictionary.

agramfort commented 4 years ago

ping @adam2392

adam2392 commented 4 years ago

Yes that totally sounds like a bug to me :P.

Additional information

If I have time I can try to submit a suggestion on how to fix it.

I think the issue is that you are using the comment texts as keys for your dictionary of annotations. So when your script finds a new comment with the same text is overwriting the previous one. Perhaps one suggestions to append the values to your keys as a list if the keys are already in the dictionary.

Ah I see this sounds like a good fix to me! Would you be willing to submit a PR and I can help review and get it merged? It seems like a few line fix to adapt the dictionary to dictionary of lists.

infinitejest commented 4 years ago

I was able to get that fixed , but it is breaking the part of the code that adds annotation to the annotation object because It is expecting single value Instead of a list. I need to give this some thought.

Andres Rodriguez

On Wed, Sep 23, 2020, 2:04 PM Adam Li notifications@github.com wrote:

Yes that totally sounds like a bug to me :P.

Additional information

If I have time I can try to submit a suggestion on how to fix it.

I think the issue is that you are using the comment texts as keys for your dictionary of annotations. So when your script finds a new comment with the same text is overwriting the previous one. Perhaps one suggestions to append the values to your keys as a list if the keys are already in the dictionary.

Ah I see this sounds like a good fix to me! Would you be willing to submit a PR and I can help review and get it merged? It seems like a few line fix to adapt the dictionary to dictionary of lists.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/8292#issuecomment-697805179, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA22CDGUPAWTGSKGQ5JYHO3SHI2BHANCNFSM4RWRTAQA .

infinitejest commented 4 years ago

Yes that totally sounds like a bug to me :P.

Additional information

If I have time I can try to submit a suggestion on how to fix it. I think the issue is that you are using the comment texts as keys for your dictionary of annotations. So when your script finds a new comment with the same text is overwriting the previous one. Perhaps one suggestions to append the values to your keys as a list if the keys are already in the dictionary.

Ah I see this sounds like a good fix to me! Would you be willing to submit a PR and I can help review and get it merged? It seems like a few line fix to adapt the dictionary to dictionary of lists.

I think i got issue resolved . Now there will be objects with same description. I needed to change some of the way you unpacked you annotations to account for the lists of annotation. This way now wach annotation has an object. An alternative way to do it is to pass the whole list to a repeated annotation element but was not sure if that will be more desirable. One thing I dont know if need fixing is that the now when you transform annotations to events it has trouble to recognize the repeated annotation instances. Let see how I can show you the code and you can test.

adam2392 commented 4 years ago

@infinitejest awesome! can you make the PR into actual mne-python? I think the current one you have is to your local copy.

infinitejest commented 4 years ago

Check if it went through. I am not very good with git.