Add append mode to IntanSplitFile building on #4070

[zm711]

@h-mayorquin,

this is my proposal on top of your PR for an append mode (without docs). If you have a reason to hate it we can close it, but this seems easy enough, no? But you know the pickle stuff better so maybe this ruins that?

may replace #4070

[h-mayorquin]

this is my proposal on top of your PR for an append mode (without docs). If you have a reason to hate it we can close it, but this seems easy enough, no? But you know the pickle stuff better so maybe this ruins that?

I am OK with this. I have the intuition that a separate class instead of a keyword argument might be safer but this seems to work. We already have a test for pickling in the test_neoextractors, can you add another entry in the test for new mode so we test both modes?

[zm711]

Yeah I'll have to find the pickle test info but I can add it.

I do think regardless of whether we do yours or this one, we should discuss whether this ought to be allowable for any intan file or only the traditional split mode which we are not checking for at the moment.

[h-mayorquin]

I do think regardless of whether we do yours or this one, we should discuss whether this ought to be allowable for any intan file or only the traditional split mode which we are not checking for at the moment.

I think that's the beauty of doing it as a separate class, users can decide where they point this to. I don't think we should be policing any further.

[zm711]

I think that's the beauty of doing it as a separate class, users can decide where they point this to. I don't think we should be policing any further.

Yeah I think I was getting hung up on your original idea about this should try to be a class that handles how the "acquisition" intended. Not checking for the timestamps is actively against the intention of that system because the system internally uses the timestamps to validate the integrity of the blocks. But it does allow people to debug a corrupted file if they need to vs just failing.

[h-mayorquin]

I am OK with this instead of my PR. I think it is an improvement as it adds another feature. Still weary of the double class inheritance but the tests are passing for the critical case so it should be fine.

[samuelgarcia]

Is this legal when double hineritance not initialize the other class ?
Just wondering..

[h-mayorquin]

Yeah, I am weary of that. But maybe it is an opportunity for us to learn something if it blows up at some point x D

[samuelgarcia]

I propose we merge this and see how long this will work.

[h-mayorquin]

Thanks for the review, @samuelgarcia

[zm711]

I think this is only okay because they both only have __init__ with no methods. So because we call the specific __init__ rather than super() we go down one path. They both use the methods of BaseRecording so the methods end up being the same. If either of them had the same methods (overwriting the base methods) then I think we would have problems. Based on my reading this is the general idea of the diamond problem and this trick shouldn't be used often. It's more that I think it is okay for the specific case because of the nature of the classes.

[zm711]

Do we want a comment in the code saying that this is slightly dangerous but we are testing it out blame zm711 if it goes wrong?

[h-mayorquin]

Do we want a comment in the code saying that this is slightly dangerous but we are testing it out blame zm711 if it goes wrong?

It is too much of a hunch to write anything that would be useful I think. If we get an error at some point we can just create a separate class and use a router in the snake_case version of the caller to decided which class we use. This could also be done with new but Sam and Alessio think that pattern might not play well with serialization which would be another danger.

[zm711]

It is too much of a hunch to write anything that would be useful I think. If we get an error at some point we can just create a separate class and use a router in the snake_case version of the caller to decided which class we use.

Fair enough. I'll leave this to @samuelgarcia / @alejoe91 now :)