Skip to content
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

Qhc 848 support iq modulation for r&s sgs100 a #872

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

flavie-lebars
Copy link
Collaborator

No description provided.

Copy link

linear bot commented Jan 15, 2025

Copy link

🔗Pullpo.io Slack channel

Copy link

pullpo-for-slack bot commented Jan 15, 2025

AI Analysis

Review:

The changes in the test file seem to be adding new fixtures and test methods, which enhances the test coverage for the QDevil QDAC2 instrument. In the rohde_schwarz/sgs100a.py file, the addition of a new property and methods related to IQ state management seems like a meaningful enhancement to the functionality. The updates in qdevil_qdac2.py for defining ramping rates as constants and validating them in the set up methods contribute to better error handling and maintainability.

Detailed file changes

(dropdown):

In tests/instruments/qdevil/test_qdevil_qdac2.py:

  • Added new fixtures qdac_out_range and waveform.
  • Updated get_square_waveform fixture to return Square(0.1, 4).
  • Added test methods: test_clear_cache, test_input_range_runcard, test_input_range_set_parameter, and test_input_range_set_parameter_enabled.

In src/qililab/instruments/rohde_schwarz/sgs100a.py:

  • Added a new property IQ_state to the SGS100ASettings class.
  • Added methods to set and get the IQ_STATE parameter in the SignalGenerator class.
  • Updated the initial_setup method to include setting the IQ_state on the device.

In src/qililab/instruments/qdevil/qdevil_qdac2.py:

  • Defined minimum and maximum ramping rates as class constants.
  • Updated the set_parameter method to check if the ramp rate is within the valid range.
  • Updated the initial_setup method to validate the ramp rate for each channel and raise a ValueError if it's out of range.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.29%. Comparing base (f0a32d2) to head (2ae8d74).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #872   +/-   ##
=======================================
  Coverage   97.28%   97.29%           
=======================================
  Files         231      231           
  Lines        8219     8232   +13     
=======================================
+ Hits         7996     8009   +13     
  Misses        223      223           
Flag Coverage Δ
unittests 97.29% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@fedonman fedonman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good for me! Great work 💯

Made a suggestion on using an existing parameter.

@@ -106,6 +116,12 @@ def set_parameter(self, parameter: Parameter, value: ParameterValue, channel_id:
else:
self.turn_off()
return
if parameter == Parameter.IQ_STATE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables hardware modulation, right? If yes, we have a Parameter.HARDWARE_MODULATION that we use in other instruments as well.

Let's verify this with measurements team.

@@ -78,6 +79,15 @@ def rf_on(self):
"""
return self.settings.rf_on

@property
def IQ_state(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the same concept, this could be modulation or hardware_modulation or similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants