Skip to content

Conversation

@Kleinjohann
Copy link
Contributor

Addresses #632

I extended the spiketrain.merge function to an arbitrary number of arguments with the interface def merge(self, *others) in order not to change anything for the case of just merging two spiketrains as suggested in #632 (comment)

I similarly extended BaseNeo.merge and BaseNeo.merge_annotations in order to be able to merge the annotations correctly.

@pep8speaks
Copy link

pep8speaks commented Feb 28, 2019

Hello @Kleinjohann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-07 08:51:12 UTC

@JuliaSprenger JuliaSprenger self-assigned this Feb 28, 2019
@Kleinjohann
Copy link
Contributor Author

I'm not too sure how merging SpikeTrainProxies is supposed to work, should it just not work and raise an error? Or should it load the SpikeTrains automatically? Or should it return a ProxyObject which will return the merged SpikeTrain when one calls its load function?

@samuelgarcia
Copy link
Contributor

Intuitivelly I would say "load automatically" then merge.
With this we could merge a list that mix SpikeTrain and SpikeTrainProxy.

kleinjohann added 3 commits March 3, 2019 22:52
…ributes always results in merge(unique list) or one single attribute if it is the same for all spiketrains. Made sure this still works if the involved spiketrains are already merged spiketrains.
…pikeTrain objects, added a unittest for this case
…into enh/merge_multiple_spiketrains

# Conflicts:
#	neo/core/spiketrain.py
@apdavison apdavison added this to the 0.8.0 milestone Apr 8, 2019
@apdavison apdavison modified the milestones: 0.8.0, 0.9.0 Sep 24, 2019
@Kleinjohann
Copy link
Contributor Author

I incorporated the changes @apdavison requested and resolved the conflicts.
Merging multiple SpikeTrain objects could also be interesting in the context of the new SpikeTrainList in #588. Something like SpikeTrainList.as_single_spiketrain would cover most of the use cases I am using this for.

@JuliaSprenger
Copy link
Member

Hi @Kleinjohann. Thank for the fixes. To me the PR looks fine. The only issue not solved is if SpiketrainProxyObjects should be automagically loaded for merging as suggested by @samuelgarcia.

@Kleinjohann
Copy link
Contributor Author

@JuliaSprenger I went with @apdavison 's remark in #652 (comment) and changed the behaviour to returning an error instead of loading automagically.

@JuliaSprenger
Copy link
Member

Ok, then we can leave the automagic loading of data discussion for another issue, since it's a rather fundamental one.

@JuliaSprenger JuliaSprenger merged commit 37f0cfc into NeuralEnsemble:master Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants