-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multichannel instrument consistency for optical spectrum analyzer #431
base: main
Are you sure you want to change the base?
Multichannel instrument consistency for optical spectrum analyzer #431
Conversation
…onsistency fix tests to adjust to new metaclass refractoring Finish optical_spectrum_analyzer channel consistency
@@ -7,7 +7,6 @@ | |||
|
|||
|
|||
from enum import IntEnum, Enum | |||
import hashlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup unused import
@@ -46,6 +45,7 @@ class Yokogawa6370(OpticalSpectrumAnalyzer): | |||
|
|||
def __init__(self, *args, **kwargs): | |||
super().__init__(*args, **kwargs) | |||
self._channel_count = len(self.Traces) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now required from metaclass. see channel
property below for range
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
=======================================
Coverage 99.14% 99.14%
=======================================
Files 91 91
Lines 9240 9253 +13
=======================================
+ Hits 9161 9174 +13
Misses 79 79
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
with pytest.raises(NotImplementedError): | ||
_ = inst.channel | ||
ch = inst.channel[0] | ||
assert isinstance(ch, ik.abstract_instruments.OpticalSpectrumAnalyzer.Channel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the channel itself is automatically implemented now and - if a single channel instrument - will forward to the parent's channel. so this needed to be changed
@@ -78,15 +72,25 @@ def test_osa_start_sweep(osa): | |||
# OSAChannel # | |||
|
|||
|
|||
def test_osa_channel_wavelength(osc): | |||
@pytest.mark.parametrize("num_ch", [1, 5]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take care of single and multichannel instruments
with expected_protocol(osa, [], []) as inst: | ||
inst._channel_count = num_ch | ||
ch = inst.channel[0] | ||
with pytest.raises(NotImplementedError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call single/mutli channel instrument through the inst.channel ProxyList function and ensure proper forwarding
with pytest.raises(NotImplementedError): | ||
ch.wavelength() | ||
with pytest.raises(NotImplementedError): | ||
inst.wavelength() # single channel instrument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the single channel function to call (directly from instrument class) and ensure it exists and raises appropriate error
|
||
|
||
def test_osa_channel_data(osc): | ||
@pytest.mark.parametrize("num_ch", [1, 5]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
@@ -31,6 +29,7 @@ | |||
|
|||
def test_channel_is_channel_class(): | |||
inst = ik.yokogawa.Yokogawa6370.open_test() | |||
assert inst._channel_count == len(inst.Traces) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ik.yokogawa.Yokogawa6370.Traces
is 5 channels long, this is provided to the proxy list. Making sure here we set this now, as it is a new variable defined in the metaclass.
seems reasonable to me! |
Sweet, there's still some cleanup to do though. |
What do you think / was your reasoning about how the |
@scasagrande Happy new year! Wanted to check in if you have any info on above question on how |
Sorry! What was my reasoning? Oh goodness I don't remember the specifics. I think I removed them from the functions on the instrument class because I wanted the concrete implementation to be in the channel specific class. |
This PR provides multichannel consistency for the optical spectrum analyzer metaclass and is associated with the discussion in #156.
It has been a while that we discussed this, but I finally started looking at it again and figured that the optical spectrum analyzer seems straight forward to get my hands on this issue. Changes in the metaclass are modeled after the function generator. Would be good to hear what you think to see if this is in the right direction. Thanks!
The only instrument dependent on this metaclass is currently the Yokogawa 6370. I have only extended the tests, but not changed the instruments test in order to ensure that there are no breaking changes.
Metaclass tests are changing a bit and I will comment in a sec on the important ones.
Let me know what you think!