Extend probe constructor #206
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #206 +/- ##
==========================================
+ Coverage 88.23% 88.52% +0.28%
==========================================
Files 10 10
Lines 1641 1690 +49
==========================================
+ Hits 1448 1496 +48
- Misses 193 194 +1
☔ View full report in Codecov by Sentry. |
src/probeinterface/probe.py
Outdated
| self.annotations = dict() | ||
| self.annotate( | ||
| name=name if name is not None else "", | ||
| serial_number=serial_number if serial_number is not None else "", | ||
| model_name=model_name if model_name is not None else "", | ||
| manufacturer=manufacturer if manufacturer is not None else "", | ||
| ) |
There was a problem hiding this comment.
I think I would prefer nothing instead of empty string when None.
|
@samuelgarcia @h-mayorquin ready to review |
src/probeinterface/probe.py
Outdated
| @@ -15,7 +15,7 @@ class Probe: | |||
|
|
|||
| """ | |||
|
|
|||
| def __init__(self, ndim=2, si_units="um"): | |||
| def __init__(self, ndim=2, si_units="um", name=None, serial_number=None, model_name=None, manufacturer=None): | |||
There was a problem hiding this comment.
I am not sure to like the idea of overloading the constructor with fixed annotation even if they are important.
When creating a Probe from scratch (wich a end user normally do not do) I think I prefer this:
probe = Probe(ndim=2)
probe.annotate(name=name)
probe.annotate(serial_number=serial_number)
probe.annotate(model_name=model_name)
probe.annotate(manufacturer=manufacturer)The discussion of which annotations are important are endless.
So lets put annotations outside the constructor.
The other solution would be to put all of then (but I do not like)
probe = Probe(ndim=2, **any_annotations)There was a problem hiding this comment.
or another solution would be to make set_* for these 4 key annotations, so that they are still annotations (i.e., saved to file too), but the have an easier API. Whaat do you think?
src/probeinterface/probe.py
Outdated
| @property | ||
| def name(self): | ||
| return self.annotations.get("name", "") | ||
|
|
||
| @property | ||
| def serial_number(self): | ||
| return self.annotations.get("serial_number", "") | ||
|
|
||
| @property | ||
| def model_name(self): | ||
| return self.annotations.get("model_name", "") | ||
|
|
||
| @property | ||
| def manufacturer(self): | ||
| return self.annotations.get("manufacturer", "") |
| assert "2.0 - Single Shank" in probe.annotations["name"] | ||
| assert "2.0 - Single Shank" in probe.model_name |
There was a problem hiding this comment.
For me name of a probe is not the model name.
A name can be something which is mainingfull for a end user : like "probeA"
There was a problem hiding this comment.
in fact this is now "model_name". The model name is the manufacturer name of the probe. Name can be anything (like ProbeA)
for more information, see https://pre-commit.ci
This PR adds and unifies the
serial_number/model_name/manufacturerannotations.The
Probe()constructor can now take these optional argument to automatically populate the annotations. This does not change the probeinteface format specification.I also unified the serial number retrieval from Open Ephys and SpikeGLX, plus additional metadata.
Fixes #203