Refactor Neuropixels 2.0 probes in preparation for single-shank probes #523
Conversation
aacuevas
left a comment
aacuevas
left a comment
There was a problem hiding this comment.
I left a couple of comments, but there are other parts where these same comments are still true.
In short, while it is in general a good abstraction effort, there are some places where separation of responsibilities is broken and the general classes include switches of the specifics, which is not a good pattern.
OpenEphys.Onix1.Design/NeuropixelsV2eProbeConfigurationDialog.cs
Outdated
Show resolved
Hide resolved
OpenEphys.Onix1.Design/NeuropixelsV2eProbeConfigurationDialog.cs
Outdated
Show resolved
Hide resolved
|
@aacuevas I have updated the implementation as we discussed, and tested it out. There is no difference in behavior, but this is definitely a cleaner separation between responsibilities, and should translate well when I add in the single-shank probe. Let me know if this looks good, and I can rebase the other branch to make use of these changes. |
- Add Neuropixels V2 probe info interfaces and implementations - Moved all probe-specific logic from the dialog to the info class in the Design library - Update all methods/classes to handle the abstract class, with easy scalability to add single-shank probes in the future - Add clone method for NeuropixelsV2eProbeGroup to ensure that dialogs can be exited without affecting settings - Update how the ChannelMap is accessed and utilized
bparks13
force-pushed
the
refactor-npx2-config
branch
from
d37589c to
5db06c3
Compare
aacuevas
left a comment
aacuevas
left a comment
There was a problem hiding this comment.
I made some comments of some things I believe should be either addressed or justified.
| 1 => offset + 40.0f, | ||
| _ => throw new ArgumentException("Invalid index given.") | ||
| }; | ||
| return Activator.CreateInstance(typeof(T), Specification, Version, Probes.Select(probe => new Probe(probe)).ToArray()) as T ?? throw new NullReferenceException($"Issue creating new probe group of type {nameof(NeuropixelsV2eProbeGroup)}."); |
There was a problem hiding this comment.
I think I understand what you're trying to do here, but this might be a case of excessive complication.
It might be easier, and way more readable, to just create a virtual (or abstract) base Clone() and have every derived override with a Clone() { return new WhateverIAm() }
Maybe I am wrong, but I don't see the advantage of using reflection here. Reflection is a powerful tool, but should not be overused.
| if (base.OpenFile<NeuropixelsV2eProbeGroup>()) | ||
| { | ||
| ProbeConfiguration = new((NeuropixelsV2eProbeGroup)ProbeGroup, ProbeConfiguration.Reference, ProbeConfiguration.Probe); | ||
| MethodInfo method = GetType().GetMethod( |
There was a problem hiding this comment.
There are several things about this method that I believe warrant some justification:
-
It is a
bool OpenFile<T>(). Why is it generic if the T is not used anywhere? -
What makes this fundamentally different from the base dialog openfile? Either is generic or not, but this seems a weird combination
-
Why all the reflection stuff?
- is 100% needed that the call to OpenAndParseConfigurationFile is done with the parent type
- Even if it is, it is easier to just do
dynamic probeGroupObj = ProbeGroup; //probeGroupObj will be of the derived type OpenAndParseConfigurationFile(probeGroupObj);In any case, I tried to go down the rabbit hole of this method, and I believe it starts to get too complex with generics at some point.
I believe the objective of all this chain of generic is to be able to call
But, would it not be easier to at some point call the methods with a Type argument, use the
object? JsonConvert.DeserializeObject(string value, Type? type, JsonSerializerSettings? settings)overload and then cast to whatever you need?So at some point you can break the generic chain and just do something like
OpenAndParseConfigurationFile(T.GetType())in some places andOpenAndParseConfigurationFile(GetType())here for example
| [Browsable(false)] | ||
| [Externalizable(false)] | ||
| [XmlElement(nameof(ProbeGroup))] | ||
| public override string ProbeGroupString |
There was a problem hiding this comment.
Does this not collide with the other PR to get json out of the xml?
I suppose it's an interim solution...
This PR was initially a part of #493; it has been split out into this PR as well so that we can review the underlying infrastructural changes independently of the addition of the single-shank Neuropixels 2.0 probes. This is prompted by #522, which would benefit from some of these changes but we do not need the single-shank probes for #522 to be implemented.
Below are the major highlights of this PR: