Add support for Hewlett Packard 8753E VNA #1004

Sionwage · 2023-12-19T21:35:01Z

I would like to add a basic HP8753E VNA support to pymeasure.

The functionality is not ready but I would like feedback as I go and there are some specifics for coding I would like assistance with finding documentation on how to handle these.

How do I handle default Prologix GPIB-USB adapter parameters to work with this device?

I found these pages that address how to handle read/write terminations and other parameters for the various adapters but I am unsure how to apply this to the prologix gpib-usb adapter I utilize.
- This issue states I shouldn't set specific adapter settings to make the instrument work:
  - Discourage the use of direct setting of interface specific properties in Instruments #815
- These pages makes it look like I should be configuring adapter settings:
  - https://pymeasure.readthedocs.io/en/latest/dev/adding_instruments/instrument.html#defining-default-connection-settings
  - https://pyvisa.readthedocs.io/en/latest/api/constants.html#pyvisa.constants.InterfaceType
I am not sure if I should be using asrl or gpib for pyvisa.constants.InterfaceType since its a serial device connecting to GPIB.

Unfortunately, it seems like if I am not defining the Prologix settings and commanding them when using the VNA, there will be unexpected behavior or it will timeout/error out.

It looks like it is addressed here but not implemented here Handle Prologix USB to GPIB pyvisa/pyvisa#112
If I utilize default Prologix GPIB-USB adapter parameters for one instrument, is the functionality to change these parameters on the fly for other instruments in place? IE, if I need '++auto 1' for one instrument but not another, with pymeasure send the command to the adapter if I communicate to alternating instruments and remember when it needs to change it?

The Update graphical.rst #112 issue makes it look like different parameters needed with the Prologix adapter for multiple instruments are not handled yet with pymeasure or pyvisa.
Are we allowed to utilize cached properties to save time on communicating?

My to-do list of actions still needed to polish this for others to use is:

Need unit tests for this class that don't rely on having the instrument. Currently my unit tests rely on having the instrument on the bench and connected. I know there is some functionality at Adding Instruments->Writing tests->Protocol tests->Test generator to create tests that don't demand the instrument be attached to the computer.
Remove adapter specific commands like my workarounds using the Prologix adapter from the class or be able to toggle the Prologix specific functions on/off when initializing the class.
Write comments for each of the functions clarifying them
Start the comments for properties with ['Get', 'Measure', 'Set', or 'Control']
May need to move the __init__ block to __new__?
Exceptions may need to use more specific exceptions from the pymeasure library
Probably need to remove the __version and if __name__=="__main__": blocks
Address other code style issues I am undoubtedly not adhering to.

BenediktBurger

Thank you very much for your contribution. I commented on your code.

A few general points to consider:

You might add yourself to the authors.txt file
Please add documentation files (see https://pymeasure.readthedocs.io/en/latest/dev/adding_instruments/instrument.html#adding-documentation) for the API documentation

Answers to your questions:

"How do I handle default Prologix parameters": You have to define them in your personal code, not in pymeasure, as pymeasure defines the parameters for the instrument communication, such that anyone might use the device independent of the adapter, see the init part.
"I am not sure if I should be using asrl or gpib for pyvisa.constants.InterfaceType since its a serial device connecting to GPIB.": gpib as that is the device connection.
"Unfortunately, it seems like if I am not defining the Prologix settings and commanding them when using the VNA, there will be unexpected behavior or it will timeout/error out.": You have to create your PrologixAdapter with the correct settings beforehand.
"If I utilize default Prologix GPIB-USB adapter parameters for one instrument, is the functionality to change these parameters on the fly for other instruments in place? IE, if I need '++auto 1' for one instrument but not another, with pymeasure send the command to the adapter if I communicate to alternating instruments and remember when it needs to change it?": You have to manage your PrologixAdapter yourself.
"Are we allowed to utilize cached properties to save time on communicating?" Right now we do not use them. In case they might be good, we could discuss it. I've seen you cache the id/serial_number, but I don't see the point of caching, as you read these data rarely.

If you have more questions, don't hesitate to ask.

pymeasure/instruments/hp/hp8753e.py

… for adding HP8753E using Python 3.12

BenediktBurger

A second round of comments.

Thanks for your work, which is already in a good state.

pymeasure/instruments/hp/__init__.py

pymeasure/instruments/hp/hp8753e.py

pyproject.toml

pymeasure/instruments/ni/daqmx.py

tests/instruments/hp/test_hp8753e.py

tests/instruments/hp/test_hp8753e_with_device.py

Sionwage · 2024-02-13T15:51:12Z

Is there a way to change the branch to merge from? I have somehow completely hosed my git clone and cannot update/sync my copied repo with the changes made in pymeasure:master since opening this.

Otherwise I'll just copy out my code, nuke my fork repo, refork the repo and create a branch before putting my code back in so I can sync upstream changes.

BenediktBurger · 2024-02-13T18:22:00Z

You cannot change the branch name to be merged from.

You can, however use different commits in the branch:

create a temporary branch, where you are at right now (and maybe the files)
delete your current branch
create a new branch with the name of the current branch (for example on the current master)
Do the changes you want (e.g. copying the files)
Push your changes to your github repo (using push force)

… Hewlett Packard Instrument Files.

codecov · 2024-04-03T18:33:25Z

Codecov Report

Attention: Patch coverage is 58.75000% with 66 lines in your changes are missing coverage. Please review.

Project coverage is 58.47%. Comparing base (fb5f3b2) to head (99a4ba2).
Report is 16 commits behind head on master.

Files	Patch %	Lines
pymeasure/instruments/hp/hp8753e.py	58.49%	66 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
+ Coverage   58.16%   58.47%   +0.30%     
==========================================
  Files         259      262       +3     
  Lines       17934    18257     +323     
==========================================
+ Hits        10432    10676     +244     
- Misses       7502     7581      +79

Flag	Coverage Δ
unittests	`58.47% <58.75%> (+0.30%)`	⬆️

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.

…st to match another one in the same folder for the docs.

…8753E:2: WARNING: Field list ends without a blank line; unexpected unindent.` https://stackoverflow.com/questions/61888521/python-sphinx-warning-definition-list-ends-without-a-blank-line-unexpected-uni

Sionwage · 2024-04-03T19:41:36Z

As I see it, I think there are two/three items still needing to get addressed before you would accept this pull request.

PrologixAdapter fixture for unit-testing the HP8753E needs to be resolved
May need more unit tests to cover the 40% or so of instrument code that isn't in a unit-test.
May need to rewrite the measurement_parameter property to use Instrument.control

BenediktBurger · 2024-04-05T12:46:32Z

May need more unit tests to cover the 40% or so of instrument code that isn't in a unit-test.

Don't worry about less than 100% patch coverage.
Tests are good, but we do not have any limit for merging pull requests.

… the docstring instead of trying to do a f-string.

Sionwage · 2024-04-18T23:06:59Z

I think I am close to done with all the requests made and just need answers to the last 3 unresolved conversations to finish.

BenediktBurger · 2024-04-24T05:45:12Z

Hey @Sionwage,
I'm in the process of reviewing, but I'll need some time, due to time constraints.
I ask for your patience.

BenediktBurger

Thanks for your contribution.

I mentioned some incosistencies and mainly documentation issues. The instrument looks already good.

I noticed quite a lot dynamic properties. Do you intend to use them as dynamic properties or is it "just in case"?

docs/api/instruments/hp/hp8753E.rst

tests/instruments/hp/test_hp8753e.py

tests/instruments/hp/test_hp8753e_with_device.py

tests/instruments/test_all_instruments.py

pymeasure/instruments/hp/hp8753e.py

…xture using the --device-address flag. Also adjusted the sleep delays for allowing the VNA to catch up.

…ing ending on a blank line

…e `test_hp8753e_with_device_scan_single`

Sionwage · 2024-05-13T16:11:45Z

Thanks for your contribution.

I mentioned some incosistencies and mainly documentation issues. The instrument looks already good.

I noticed quite a lot dynamic properties. Do you intend to use them as dynamic properties or is it "just in case"?

There are different configurations for the 8753E so this allows others to override the defaults set for their instrument.

Sionwage · 2024-05-13T16:13:54Z

Not sure how to resolve WARNING: Field list ends without a blank line; unexpected unindent.

Otherwise I think everything has been addressed now.

BenediktBurger · 2024-05-14T12:59:51Z

tests/instruments/hp/test_hp8753e_with_device.py

+            prologix = PrologixAdapter(resource_name=prologix_address, visa_library="@py", auto=1)
+            # need to ensure `eot_enable` is set to zero otherwise you will have to read twice to
+            # get rid of the extra new line character
+            prologix.write("++eot_enable 0")
+            vna = HP8753E(adapter=prologix.gpib(int(gpib_address)))


By the way, you do not need to create a PrologixAdapter and then call gpib.
The following two options result in the same behavior:

temp_adapter = PrologixAdapter(resource_name) adapter = temp_adapter.gpib(gpib_address) # or adapter = PrologixAdapter(resource_name, gpib_address)

BenediktBurger · 2024-05-14T13:01:03Z

tests/instruments/hp/test_hp8753e_with_device.py

+# this test will wear out the mechanical relays and should be run only as necessary
+@pytest.mark.skip


You can add a reason to the skip:

Suggested change

# this test will wear out the mechanical relays and should be run only as necessary

@pytest.mark.skip

@pytest.mark.skip(reason="This test will wear out the mechanical relays and should be run only as necessary")

tests/instruments/hp/test_hp8753e_with_device.py

BenediktBurger

Some comments.

Yes, it comes good together, we're almost done.

BenediktBurger · 2024-05-14T13:08:28Z

pymeasure/instruments/hp/hp8753e.py

+    :param max_power: Optional set the maximum validator for `HP8753E.power` on initialization
+        (float in dBm).
+    """


I think that is the culprit for the warning.
For such a list, sphinx expects the last line to start with :param or an empty line, so you could do:

either move (float ind dBm). to the end of the previous line 53 (such that the last line starts with :param...

or add an empty line before the end of the docstring.

We have a line length limit of 100, so the first option should work.

BenediktBurger · 2024-05-14T13:11:37Z

CHANGES.rst

Please revert the changes in this file.

PRs should be based on the master branch and merge there.

The v0.14 branch is there to write the changelog for the next release.

BenediktBurger reviewed Dec 20, 2023

View reviewed changes

Sionwage pushed a commit to Sionwage/pymeasure that referenced this pull request Dec 21, 2023

Implementing corrections for pull request pymeasure#1004 in pymeasure…

6e23d2b

… for adding HP8753E using Python 3.12

Sionwage marked this pull request as draft December 26, 2023 17:23

BenediktBurger requested changes Jan 5, 2024

View reviewed changes

Wiped branch and branched from the current pymeasure master to add my…

fd80a82

… Hewlett Packard Instrument Files.

Sionwage force-pushed the master branch from c6e894f to fd80a82 Compare April 3, 2024 18:30

Sionwage added 5 commits April 3, 2024 13:39

Fixed flake8 errors.

df23e7c

Removed "HP8753E" replaced with self or nothing.

6fbd987

Fixed flake8 linting errors.

63fd024

Fixed title overline too short and added extra new line for hp8753E.r…

6b9db33

…st to match another one in the same folder for the docs.

Fixing sphinx document error for `pymeasure.instruments.hp.hp8753e.HP…

9798011

…8753E:2: WARNING: Field list ends without a blank line; unexpected unindent.` https://stackoverflow.com/questions/61888521/python-sphinx-warning-definition-list-ends-without-a-blank-line-unexpected-uni

Sionwage marked this pull request as ready for review April 4, 2024 15:01

Sionwage added 3 commits April 18, 2024 17:47

Cleaned up HP8753E.measuring_parameter

37ca73e

Fixed the nonetype split error for measuring_parameters by hardcoding…

a881091

… the docstring instead of trying to do a f-string.

Lost IFBW somewhere?

5b2540d

Sionwage requested a review from BenediktBurger April 18, 2024 23:07

BenediktBurger requested changes Apr 25, 2024

View reviewed changes

Sionwage and others added 6 commits April 26, 2024 17:23

Updates requested by BenediktBurger

eabfe02

Added the pytest skip back for "test_hp8753e_with_device.py"

4c0e3fa

Add changelog

81bf9ca

Rewrote the tests to work with the Prologix Adapter or pyvisa test fi…

94820ab

…xture using the --device-address flag. Also adjusted the sleep delays for allowing the VNA to catch up.

Merge remote-tracking branch 'upstream/v0.14.0_release'

ddb25a9

Updated AUTHORS.txt

c358c73

Sionwage added 3 commits May 13, 2024 10:54

Fixed HP8753E Test for set_single_frequency_scan and HP8753E docstr…

1eff61c

…ing ending on a blank line

Fixed incorrectly written assertion and used the data variable for th…

68085b0

…e `test_hp8753e_with_device_scan_single`

Fix linting errors

99a4ba2

BenediktBurger reviewed May 14, 2024

View reviewed changes

BenediktBurger requested changes May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Hewlett Packard 8753E VNA #1004

Add support for Hewlett Packard 8753E VNA #1004

Sionwage commented Dec 19, 2023 •

edited

BenediktBurger left a comment

BenediktBurger left a comment

Sionwage commented Feb 13, 2024

BenediktBurger commented Feb 13, 2024

codecov bot commented Apr 3, 2024 •

edited

Sionwage commented Apr 3, 2024 •

edited

BenediktBurger commented Apr 5, 2024

Sionwage commented Apr 18, 2024

BenediktBurger commented Apr 24, 2024

BenediktBurger left a comment

Sionwage commented May 13, 2024

Sionwage commented May 13, 2024

BenediktBurger May 14, 2024

BenediktBurger May 14, 2024

BenediktBurger left a comment

BenediktBurger May 14, 2024

BenediktBurger May 14, 2024

		# this test will wear out the mechanical relays and should be run only as necessary
		@pytest.mark.skip

	# this test will wear out the mechanical relays and should be run only as necessary
	@pytest.mark.skip
	@pytest.mark.skip(reason="This test will wear out the mechanical relays and should be run only as necessary")

Add support for Hewlett Packard 8753E VNA #1004

Are you sure you want to change the base?

Add support for Hewlett Packard 8753E VNA #1004

Conversation

Sionwage commented Dec 19, 2023 • edited

I would like to add a basic HP8753E VNA support to pymeasure.

My to-do list of actions still needed to polish this for others to use is:

BenediktBurger left a comment

Choose a reason for hiding this comment

BenediktBurger left a comment

Choose a reason for hiding this comment

Sionwage commented Feb 13, 2024

BenediktBurger commented Feb 13, 2024

codecov bot commented Apr 3, 2024 • edited

Codecov Report

Sionwage commented Apr 3, 2024 • edited

BenediktBurger commented Apr 5, 2024

Sionwage commented Apr 18, 2024

BenediktBurger commented Apr 24, 2024

BenediktBurger left a comment

Choose a reason for hiding this comment

Sionwage commented May 13, 2024

Sionwage commented May 13, 2024

BenediktBurger May 14, 2024

Choose a reason for hiding this comment

BenediktBurger May 14, 2024

Choose a reason for hiding this comment

BenediktBurger left a comment

Choose a reason for hiding this comment

BenediktBurger May 14, 2024

Choose a reason for hiding this comment

BenediktBurger May 14, 2024

Choose a reason for hiding this comment

Sionwage commented Dec 19, 2023 •

edited

codecov bot commented Apr 3, 2024 •

edited

Sionwage commented Apr 3, 2024 •

edited