Harmonise instrument name definition pattern, consistently name the instrument connection argument "adapter" #659

BenediktBurger · 2022-08-02T10:54:41Z

Fixes #658 and adjustes the documentation accordingly.

Verify that the whole adding_instruments documentation is up to date.
Create a follow up Issue for changing all instruments. (correct the super invocation in Keysight N7776, LakeShore421), maybe together with the change to a "SCPI" mixin.

bilderbuchi · 2022-08-02T12:29:32Z

I'm actually not sure if we should always put the default name into the constructor signature, as it pollutes that (and displayed docstrings, presumably) with a pretty long entry that is not that central.
I'd prefer we use the setdefault to set a reasonable default name (i.e. the model identifier) in the constructor, and allow the user to easily override it with a setup-specific name if desired:

def test_name_setting():
    class NamedInstrument(Instrument):
        def __init__(self, adapter, **kwargs):
            kwargs.setdefault('name', 'Default Instrument')
            super().__init__(adapter, **kwargs)

    adapter = FakeAdapter()
    instr = NamedInstrument(adapter)
    assert instr.name == 'Default Instrument'
    # Let's use a user-specific name (e.g. to disambiguate multiple identical devices)
    instr2 = NamedInstrument(adapter, name='Vacuum Frobnicator')
    assert instr2.name == 'Vacuum Frobnicator'

This way the signature of NamedInstrument stays clean, but we remain as flexible as possible.
What do you think?

BenediktBurger · 2022-08-02T12:46:53Z

Normally I prefer to know, what values I might change and what the default is. setdefault hides the possibility to rename the device.
As changing the name is rarely used (almost no instrument allowed for that), I'm fine with hiding the renaming option.

One caveat though: The documentation does not hand the kwargs to the instrument, but to the adapter in case of a custom adapter!
We should take care of that as well.
We might even remove that example, as custom adapters are rarely necessary and should not be used, instead read/write.

Once I'm remodeling all instruments. Should we name the adapter consistently adapter? In some instruments it is called "resourceName", which is taken from the documentation.

bilderbuchi · 2022-08-02T13:03:44Z

One caveat though: The documentation does not hand the kwargs to the instrument, but to the adapter in case of a custom adapter!

I'm not sure what you mean?

Once I'm remodeling all instruments. Should we name the adapter consistently adapter? In some instruments it is called "resourceName", which is taken from the documentation.

I think this comes from pyvisa nomenclature, where it's "resource name". I'm not sure. I guess in 95% of cases people supply a resource name string, so that would be a more fitting name. However, if people pass an Adapter instance, resourceName is a weird name. :-P -- adapter is delightfully ambiguous 😒
@msmttchr what do you think?

adapter is also consistent with Instrument. Renaming the Instrument posarg will probably break a lot of code I suspect.

msmttchr · 2022-08-02T18:20:49Z

Once I'm remodeling all instruments. Should we name the adapter consistently adapter? In some instruments it is called "resourceName", which is taken from the documentation.

I think this comes from pyvisa nomenclature, where it's "resource name". I'm not sure. I guess in 95% of cases people supply a resource name string, so that would be a more fitting name. However, if people pass an Adapter instance, resourceName is a weird name. :-P -- adapter is delightfully ambiguous 😒 @msmttchr what do you think?

adapter is also consistent with Instrument.

I agree, in my opinion adapter is the correct name.

BenediktBurger · 2022-08-03T11:02:51Z

I made the talked about changes (to adapter and using kwargs).

There are some doctest errors in the documentation though.

docs/dev/adding_instruments.rst

pymeasure/instruments/activetechnologies/AWG401x.py

pymeasure/instruments/agilent/agilent33521A.py

docs/dev/adding_instruments.rst

pymeasure/instruments/attocube/anc300.py

bilderbuchi · 2022-08-03T13:44:40Z

I'm actually not sure if we should always put the default name into the constructor signature, as it pollutes that (and displayed docstrings, presumably) with a pretty long entry that is not that central. I'd prefer we use the setdefault to set a reasonable default name (i.e. the model identifier) in the constructor, and allow the user to easily override it with a setup-specific name if desired:
def test_name_setting():
    class NamedInstrument(Instrument):
        def __init__(self, adapter, **kwargs):
            kwargs.setdefault('name', 'Default Instrument')
            super().__init__(adapter, **kwargs)

    adapter = FakeAdapter()
    instr = NamedInstrument(adapter)
    assert instr.name == 'Default Instrument'
    # Let's use a user-specific name (e.g. to disambiguate multiple identical devices)
    instr2 = NamedInstrument(adapter, name='Vacuum Frobnicator')
    assert instr2.name == 'Vacuum Frobnicator'
This way the signature of NamedInstrument stays clean, but we remain as flexible as possible. What do you think?

Please also add that test to test_instrument.py, it's good to have tests that define best practices, so they constantly get checked/confirmed.

bilderbuchi · 2022-08-03T13:56:09Z

I don't know what's up with the doctest, first I thought that it's because we swap in FakeInstrument in adding_instruments.rst L89 (to enable doctesting), but I can't seem to reproduce this locally, and the error message does not make sense to me. Let's see if it's a transient or appears again.

pymeasure/instruments/advantest/advantestR3767CG.py

BenediktBurger · 2022-08-03T16:12:53Z

Thanks for your reviews. I implemented the changes.

I also implemented a test whether a named argument removes the corresponding entry from kwargs.

bilderbuchi · 2022-08-03T16:57:37Z

I think we're good now! Only the doctests are still broken (and a small linting issue remains). 😿 I'll pull and see if I can figure it out locally.

Afterwards, we should check/update all open PRs for the new usage (adapter and name handling). I don't know how to meaningfully automate those checks. 😞

docs/dev/adding_instruments.rst

pymeasure/instruments/activetechnologies/AWG401x.py

pymeasure/instruments/agilent/agilent33521A.py

BenediktBurger · 2022-08-03T17:22:49Z

Afterwards, we should check/update all open PRs for the new usage (adapter and name handling). I don't know how to meaningfully automate those checks.

Testing with something like

def test_args():
    adapter = FakeAdapter()
    instr = NewInstrument(adapter=adapter, **{'name': "test successful"})
    assert NewInstrument.name == "test successful"

This test verifies, whether the first argname is adapter, because if it is not adapter, it won't receive a value and will raise an error.
It also verifies, that the name argument is present somehow.

bilderbuchi · 2022-08-03T17:38:49Z

This test verifies, whether the first argname is adapter, because if it is not adapter, it won't receive a value and will raise an error.
It also verifies, that the name argument is present somehow.

Sorry, I was unclear. Yeah such a test tests what we need, but it does not do that (dynamically) for any instruments changed in a PR. Adding this same test to all instruments individually seems wasteful. I think we could enumerate all instruments, and feed those to that test, though! (or one test for the adapter, one for the name) :-)

BenediktBurger · 2022-08-03T18:14:49Z

Sorry, I was unclear. Yeah such a test tests what we need, but it does not do that (dynamically) for any instruments changed in a PR. Adding this same test to all instruments individually seems wasteful. I think we could enumerate all instruments, and feed those to that test, though! (or one test for the adapter, one for the name) :-)

I wrote a test, which searches for all instruments in pymeasure and does this test.
The problem is, however, that the inits of some devices fail, because they need some initial communication (adapter.connection.clear or setting/getting something etc.) or because they require some obligatory arguments (port, baud_rate...), which we do not supply.
Right now we pass 58 and fail 26.

bilderbuchi · 2022-08-03T18:29:42Z

I'd say push the test, if there are obvious fixes make them, if we need to add something to FakeAdapter (if only that some instrument has something to call) we can do that.
Finally, assuming you store the found instruments in a list for parameterize, we can make a filter list where we can skip (pytest.skipif or mark xfail with strict mode) an instrument test if it's on the list (separate for a test for name and a test for adapter?). That way we have 1-2 filter list(s) we can slowly work to reduce. What do you say?

bilderbuchi · 2022-08-03T18:34:00Z

In general, that list of all instruments (module-level ALL_INSTRUMENTS?) could come in handy for further API tests like this. :-)

tests/instruments/test_all_instruments.py

bilderbuchi · 2022-08-03T19:44:49Z

Awesome addition with that test, we can work with that!

docs/dev/adding_instruments.rst

msmttchr · 2022-08-03T20:54:00Z

(This comment is related to
#659 (comment), but from mobile phone rendering I am not sure this obvious)
I am not entirely clear what is the advantage, I think the name of the instrument is something fixed and defined by instrument driver writer. Why do we want to allow user to override it ?
If user wants to do this, he can still do something like inst.name = "Something".
On the other hand, implementating this suggestion has triggered a lot of changes in all instruments making, in my opinion, the code more obscure

BenediktBurger · 2022-08-03T21:04:37Z

I am not entirely clear what is the advantage, I think the name of the instrument is something fixed and defined by instrument driver writer. Why do we want to allow user to override it ? If user wants to do this, he can still do something like inst.name = "Something". On the other hand, implementating this suggestion has triggered a lot of changes in all instruments making, in my opinion, the code more obscure

For a user yes, instr.name = "xy" works, but for subclassing, you can just hand the name to the parent class as if it were Instrument itself.

bilderbuchi · 2022-08-04T19:17:08Z

I am not entirely clear what is the advantage, I think the name of the instrument is something fixed and defined by instrument driver writer. Why do we want to allow user to override it ?

One use case for users giving custom names is when you have an experiment with several identical devices, e.g. several power supplies controlling coil current, bias voltage, excitation voltage (random example), then you would name the instances as such so that it's clearer/more convenient to see further down (e.g. in logging, GUI use, etc).
E.g. I have previously written code that uses the instrument name when naming a data file name or data column name.

The other use case is when subclassing, as already mentioned.

BenediktBurger · 2022-08-04T20:47:51Z

With that, I think, we're good to go.

bilderbuchi · 2022-08-05T07:49:01Z

With that, I think, we're good to go.

Me too.
@msmttchr, agree to merge this?

BenediktBurger · 2022-11-21T18:43:08Z

Another example of issues with the name, see #698 (comment)

In order to ensure more easy subclassing, I'm for enabling all instruments to allow a name argument. This PR is already done, so I propse to use it.
If we decide to change to name="Some funny instrument" argument in the init, we still can do it without breaking anything. The work to do this change will be almost the same with or without this PR merged.
However, if you prefer, I would do the work to change all instruments to super().__init__(adapter, name="My name", **kwargs).

Is that PR #698 enough for you, to see name as changeable, @msmttchr?

If not, we can close the PR (the code still remains and could be used later on).
@bilderbuchi

msmttchr · 2022-11-21T19:25:34Z

Another example of issues with the name, see [#698 (comment)](#698 (comment)

Is that PR #698 enough for you, to see name as changeable, @msmttchr?

Yes, even though I would do as class attribute as I mentioned previously. On the other hand, pretty much all users prefer to have it as parameter, so for me it's ok to have as parameter.

BenediktBurger · 2022-11-22T09:35:46Z

Now remains the question of the documentation:

Do we want only one style (init parameter or kwargs.setdefault), do we allow both, but prefer one or do we allow both?
In the first two questions: which one?

AidenDawn · 2022-11-22T13:12:25Z

Something I remember from #719 is that if ATSBase and Instrument now have a name during the test, they don't need to be skipped in test_all_instruments.py anymore. I've given this a quick test in your branch, but it would be worth double-checking

bilderbuchi · 2022-11-22T19:16:37Z

Now remains the question of the documentation:

Do we want only one style (init parameter or kwargs.setdefault), do we allow both, but prefer one or do we allow both? In the first two questions: which one?

I have a slight preference for the "init parameter" way (as Casper said we don't have that much clutter in the signature especially when includeSCPI should disappear, and it's a bit clearer that this option is there).
The prefernce is not strong, so I would also be ok with the setdefault way if others prefer that.
Whatever we choose, we should harmonize on that one in the codebase and docs (which will effectively lead by example), but not forbid the other, imo

BenediktBurger · 2022-11-22T19:45:59Z

I prefer the init parameter as well, but I'm reluctant to touch all the instruments.

What do you think:

We reduce this PR to make the documentation only (regarding init parameter).
We introduce the SCPIMixin (without yet removing includeSCPI).
When we switch all instruments from includeSCPI to a SCPIMixin, we have to touch all of them (without introducing any other code of discussion), such that we can do both (SCPI and init parameter) in one go.

EDIT:
correct super invocation in Keysight N7776, LakeShore421

bilderbuchi · 2022-11-22T20:07:41Z

Excellent suggestion, agreed, that's better!

BenediktBurger · 2022-11-22T21:59:46Z

Divide and conquer 😁

I have learnt my lesson of large PRs with too little talk about the principles first.

…trument init.

…ents. This catches new instrument PRs.

BenediktBurger · 2022-11-24T17:11:36Z

As discussed, I stored the changes in a local branch (if we should need them) and started from scratch:

Documentation is changed to the new style
All instruments are tested for accepting a "name" parameter. All currently present instruments are exempt (for future rework). The scope of the tests is to force new instruments to use that scheme.

Should we add a change note? It only affects new instrument creators.

bilderbuchi

LGTM, with some restructuring suggestions for the tests.

tests/instruments/test_all_instruments.py

bilderbuchi · 2022-11-25T20:39:45Z

Should we add a change note? It only affects new instrument creators.

Why not. A hint about the new best practice could be useful.

When I compile the changelog, I mostly go by PR titles and what I remember from the PRs. If there are details that should or must be in the changelog, it's good to add them with the PR, at least for us maintainers -- there's less potential that something is missed. This primarily concerns "core"-touching PRs, for instruments it's straightforward enough most of the time.

BenediktBurger · 2022-11-25T22:15:31Z

Thanks for the suggestions.

Rewording of variable names. Change note added. ATSBase has a default name.

docs/dev/adding_instruments.rst

BenediktBurger · 2022-11-28T22:17:55Z

As you both had only these minor (unrelated) changes, we can merge, I think.

msmttchr · 2022-11-28T22:21:32Z

As you both had only these minor (unrelated) changes, we can merge, I think.

I agree

msmttchr

Good for me

bilderbuchi

LGTM

BenediktBurger force-pushed the fix-658 branch from 1f6c441 to 4ff616e Compare August 2, 2022 12:29

BenediktBurger force-pushed the fix-658 branch from 4ff616e to 6f12dde Compare August 3, 2022 10:43

bilderbuchi requested changes Aug 3, 2022

View reviewed changes

bilderbuchi reviewed Aug 3, 2022

View reviewed changes

pymeasure/instruments/advantest/advantestR3767CG.py Outdated Show resolved Hide resolved

bilderbuchi changed the title ~~Fix 658~~ Harmonise instrument name definition pattern, consistently name the instrument connection argument "adapter" Aug 3, 2022

bilderbuchi requested changes Aug 3, 2022

View reviewed changes

docs/dev/adding_instruments.rst Outdated Show resolved Hide resolved

pymeasure/instruments/activetechnologies/AWG401x.py Outdated Show resolved Hide resolved

pymeasure/instruments/agilent/agilent33521A.py Outdated Show resolved Hide resolved

bilderbuchi reviewed Aug 3, 2022

View reviewed changes

tests/instruments/test_all_instruments.py Outdated Show resolved Hide resolved

bilderbuchi reviewed Aug 3, 2022

View reviewed changes

tests/instruments/test_all_instruments.py Outdated Show resolved Hide resolved

msmttchr reviewed Aug 3, 2022

View reviewed changes

docs/dev/adding_instruments.rst Outdated Show resolved Hide resolved

BenediktBurger force-pushed the fix-658 branch from a141664 to 4f87ea7 Compare August 4, 2022 17:46

BenediktBurger mentioned this pull request Nov 20, 2022

Instrument's classes #416

Open

BenediktBurger added 2 commits November 24, 2022 17:45

Documentation adjusted to use name="Instrument name" parameter in ins…

d9e0679

…trument init.

Test for all instruments added with exception for all current instrum…

7f884fb

…ents. This catches new instrument PRs.

BenediktBurger force-pushed the fix-658 branch from f411648 to 7f884fb Compare November 24, 2022 17:07

bilderbuchi approved these changes Nov 25, 2022

View reviewed changes

Implemented changes of code review.

a2fe9d2

Rewording of variable names. Change note added. ATSBase has a default name.

BenediktBurger requested a review from bilderbuchi November 28, 2022 07:45

msmttchr reviewed Nov 28, 2022

View reviewed changes

docs/dev/adding_instruments.rst Show resolved Hide resolved

bilderbuchi reviewed Nov 28, 2022

View reviewed changes

docs/dev/adding_instruments.rst Show resolved Hide resolved

Blank line inserted and indentation fixed.

796cd23

BenediktBurger force-pushed the fix-658 branch from 9450fe6 to 796cd23 Compare November 28, 2022 17:08

msmttchr approved these changes Nov 28, 2022

View reviewed changes

A few manufacturer were missing in the init file.

cae740a

bilderbuchi approved these changes Nov 29, 2022

View reviewed changes

BenediktBurger merged commit 4aea01d into pymeasure:master Nov 29, 2022

BenediktBurger deleted the fix-658 branch November 29, 2022 21:01

BenediktBurger mentioned this pull request Nov 30, 2022

Many instruments miss name argument #658

Closed

Harmonise instrument name definition pattern, consistently name the instrument connection argument "adapter" #659

Harmonise instrument name definition pattern, consistently name the instrument connection argument "adapter" #659

Conversation

BenediktBurger commented Aug 2, 2022 • edited

bilderbuchi commented Aug 2, 2022 • edited

BenediktBurger commented Aug 2, 2022

bilderbuchi commented Aug 2, 2022

msmttchr commented Aug 2, 2022

BenediktBurger commented Aug 3, 2022

bilderbuchi commented Aug 3, 2022 • edited

bilderbuchi commented Aug 3, 2022 • edited

BenediktBurger commented Aug 3, 2022

bilderbuchi commented Aug 3, 2022 • edited

BenediktBurger commented Aug 3, 2022

bilderbuchi commented Aug 3, 2022

BenediktBurger commented Aug 3, 2022

bilderbuchi commented Aug 3, 2022 • edited

bilderbuchi commented Aug 3, 2022

bilderbuchi commented Aug 3, 2022

msmttchr commented Aug 3, 2022 • edited

BenediktBurger commented Aug 3, 2022

bilderbuchi commented Aug 4, 2022

BenediktBurger commented Aug 4, 2022

bilderbuchi commented Aug 5, 2022

BenediktBurger commented Nov 21, 2022

msmttchr commented Nov 21, 2022

BenediktBurger commented Nov 22, 2022

AidenDawn commented Nov 22, 2022

bilderbuchi commented Nov 22, 2022

BenediktBurger commented Nov 22, 2022 • edited

bilderbuchi commented Nov 22, 2022

BenediktBurger commented Nov 22, 2022

BenediktBurger commented Nov 24, 2022

bilderbuchi left a comment

Choose a reason for hiding this comment

bilderbuchi commented Nov 25, 2022

BenediktBurger commented Nov 25, 2022

BenediktBurger commented Nov 28, 2022

msmttchr commented Nov 28, 2022

msmttchr left a comment

Choose a reason for hiding this comment

bilderbuchi left a comment

Choose a reason for hiding this comment

BenediktBurger commented Aug 2, 2022 •

edited

bilderbuchi commented Aug 2, 2022 •

edited

bilderbuchi commented Aug 3, 2022 •

edited

bilderbuchi commented Aug 3, 2022 •

edited

bilderbuchi commented Aug 3, 2022 •

edited

bilderbuchi commented Aug 3, 2022 •

edited

msmttchr commented Aug 3, 2022 •

edited

BenediktBurger commented Nov 22, 2022 •

edited