[NWBIO] Add a warning to avoid signals with the same name #1054
Conversation
JuliaSprenger
changed the title
|
Hi @legouee, |
JuliaSprenger
force-pushed
the
Convert_Data2
branch
from
afde97a to
db25dd8
Compare
|
Hi @legouee I rebased your branch on the current master. Can you check all your changes are still as you intended them? |
|
Hi @JuliaSprenger Thank you very much for your review. I added your comments to the code. |
neo/io/nwbio.py
Outdated
| signal.name = "%s %s %i" % (signal.name, segment.name, i) | ||
| logging.warning("Warning signal name exists. New name: %s" % (signal.name)) | ||
| else: | ||
| signal.name = "%s : analogsignal%d %i" % (segment.name, i, i) |
There was a problem hiding this comment.
Is it intentional that you compose the names differently for existing and non-existing name attributes? Wouldn't it make sense to always use a 'segment signal idx' ordering in the name?
There was a problem hiding this comment.
and the same of course also for events.
There was a problem hiding this comment.
Yes, that was intentional as I ran into problems with the signal name and the event name when testing with NWB files.
There was a problem hiding this comment.
Yes, I understand why you want to re-compose the signal name in some cases. I was rather wondering if it would make sense for both names (newly generated, as well as composed) to follow a similar structure, e.g. consist of 3 parts: 1st the segment info, 2nd the signal info (potentially the old signal name) and 3rd the enumeration. This would make the signal names more consistent independent if a name already existed previously or not.
There was a problem hiding this comment.
OK. I think you are right. It will be more consistent if the names have a similar structure.
I just made the changes.
legouee
force-pushed
the
Convert_Data2
branch
from
3a40053 to
8f28a23
Compare
|
Looks good to me now. Thanks for the fix! |
3 participants
No description provided.