Return created parameter from add_parameter

[jenshnielsen]

This means that you will be able to do

        self.resolution = self.add_parameter(
            "resolution",
            get_cmd="VOLT:DC:RES?",
            get_parser=float,
            set_cmd=self._set_resolution,
            label="Resolution",
            unit="V",
        )
        """Parameter to control voltage resolution """

which means that you get the benefit of the self.resolution = Parameter(..., instrument=self) form without having to remember to pass the instrument to Parameter

In documentation this currently (with the qcodes extension) looks like this:

Currently it looks like this

If we go this way we need to

• [ ] Update docs and examples to reflect this
• [ ] Add tests

[codecov[bot]]

Codecov Report

Merging #4412 (220df47) into master (220df47) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 220df47 differs from pull request most recent head 1881499. Consider uploading reports for the commit 1881499 to get more accurate results

@@           Coverage Diff           @@
##           master    #4412   +/-   ##
=======================================
  Coverage   68.24%   68.24%           
=======================================
  Files         339      339           
  Lines       31781    31781           
=======================================
  Hits        21688    21688           
  Misses      10093    10093           

[jenshnielsen]

the only question i have is what happens with docstring argument of add_parameter and the docstring that comes right after the attribute declaration - the latter takes over, right? in any case, this probably deserves a separate PR anyway to clarify what method of providing a docstring for a parameter to use when and how.

As I understand it the upshot of this is that

• The docstring after is used for sphinx (it does not run the code so it cannot know that the docstring arg is assigned to __doc__ Without that sphinx will simply skip the attribute
• The docstring below the attribute is not assigned to __doc__ so we need to do that manually. Perhaps some introspection hack could make this possible to get from the sphinx docstring but it does not seems worth it

[astafan8]

• The docstring after is used for sphinx (it does not run the code so it cannot know that the docstring arg is assigned to doc Without that sphinx will simply skip the attribute
• The docstring below the attribute is not assigned to doc so we need to do that manually. Perhaps some introspection hack could make this possible to get from the sphinx docstring but it does not seems worth it

so we are bound to have to duplicate the parameter docstrings? :( one for shpinx and another one for e.g. help(..) in vs code, jupyter, etc?

[jenshnielsen]

so we are bound to have to duplicate the parameter docstrings? :( one for shpinx and another one for e.g. help(..) in vs code, jupyter, etc?

I am afraid so unless we come up with a hack of some sort

[jenshnielsen]

@astafan8 Does not look like there is a way to get the attribute docstring at runtime https://stackoverflow.com/questions/55672640/how-can-i-get-doc-string-of-the-class-attribute-in-python

[mgunyho]

Hi, I came across this PR and would like to comment.

I see the benefit of self.param_name = ... for autocompletion and static analysis, and I think it should be done that way. However, I don't like that with this change, you have to type the parameter name twice. This feels redundant and can cause problems with e.g. typos. Typos can be detected at runtime by checking that the parameter name is correct in setattr, but still, it feels wrong.

I also think that having the dosctring as add_parameter(..., docstring="...") is better. Having the docstring as a separate string looks strange and feels a bit too magical to me. Having to write the docstring twice would be really bad. Do I understand correctly that Sphinx can only parse the separately added string? Would it be possible to do this using a decorator somehow? Or using ast as suggested in #4458.

[jenshnielsen]

I see the benefit of self.param_name = ... for autocompletion and static analysis, and I think it should be done that way. However, I don't like that with this change, you have to type the parameter name twice. This feels redundant and can cause problems with e.g. typos. Typos can be detected at runtime by checking that the parameter name is correct in setattr, but still, it feels wrong.

I agree this is suboptimal and ideally, we really should get away from having to give name which has always been strange and ugly looking. This may be possible using inspection or by implementing parameters using the descriptor protocol. Regardless I do feel like the gain of making parameters static outweighs the issue by a significant margin

I also think that having the dosctring as add_parameter(..., docstring="...") is better. Having the docstring as a separate string looks strange and feels a bit too magical to me. Having to write the docstring twice would be really bad. Do I understand correctly that Sphinx can only parse the separately added string? Would it be possible to do this using a decorator somehow? Or using ast as suggested in https://github.com/QCoDeS/Qcodes/issues/4458.

The thing is that having a string following an attribute is a well established thing with support in python and in Sphinx which experienced python users will know about but the docstring argument is something that qcodes invented with no support elsewhere.

[mgunyho]

I do feel like the gain of making parameters static outweighs the issue by a significant margin

I agree, having to type the name twice is not too bad. But the docstring is too much IMO.

having a string following an attribute is a well established thing with support in python and in Sphinx which experienced python users will know about

Interesting, I haven't seen this before and was not aware of this, maybe I'm not an experienced Python user then :P

But in any case, I think the runtime availability of docstrings is very important. As mentioned in the Stackoverflow link above (and see also discussion on Python mailing list, PEP 257), attribute docstrings are ignored by the the Python interpreter and will never be available at runtime.

Maybe one option to consider would then be to initialize parameters using a decorator, something like

class MyInstrument(...):

    @parameter(
    	unit="V",
    	label="Resolution",
    	# not sure if set_cmd/get_cmd should go here, or should we have @frequency.setter?
    )
    def resolution(self):
    	"""
    	Parameter to control voltage resolution
    	"""

Which I guess is pretty similar to the descriptor idea. This would be quite a major change, but it would solve all of the problems; autocomplete and documentation work while not having to write anything twice.

[mgunyho]

Hmm, maybe this is not perfect either, you can't access self in the decorator.

[jenshnielsen]

@mgunyho Thanks for your input. Good to know that you use the runtime docstring. I had not seem many users make use of it. I will investigate if there are ways to write a sphinx extension that parses that our when generating the docs.

I will also see if there is a way for the parameter to get its way with introspection

[jenshnielsen]

A few random thoughts that came up on this

• Perhaps we can generate the external docstring for the attribute automatically in a precommit hook?
• If we can get sphinx to document the attributes even without docs it should be possible to write a plugin that takes out the docstring

[jenshnielsen]

Spend a bit of time today exploring what is possible and found out that Sphinx will document attributes if they have a type annotation. This will remove the need to include a docstring to have a parameter documented which was one of the concerns raised above.

[codecov[bot]]

Codecov Report

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

:exclamation: No coverage uploaded for pull request base (main@182a98c). Click here to learn what that means.

:exclamation: Current head c0100e1 differs from pull request most recent head 1266b49. Consider uploading reports for the commit 1266b49 to get more accurate results

Files	Patch %	Lines
src/qcodes/extensions/refactor.py	0.00%	60 Missing :warning:
src/qcodes/instrument_drivers/rohde_schwarz/ZNB.py	0.00%	26 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4412   +/-   ##
=======================================
  Coverage        ?   66.34%           
=======================================
  Files           ?      352           
  Lines           ?    30462           
  Branches        ?        0           
=======================================
  Hits            ?    20210           
  Misses          ?    10252           
  Partials        ?        0           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jenshnielsen]

I also experimented with looking up the name from the previous stack frame using some very hacky code. While this is definitely possible I am not sure if I like this better than having to write the name twice

[jenshnielsen]

An alternative could be to overwrite __setattr__ to validate that the attribute name and the parameter short name are the same. This would at least catch any issue at runtime but also make it impossible to assign a parameter to an alias attribute name which I don't think we want.

[jenshnielsen]

This may also be interesting to improve the way parameters are documented https://www.sphinx-doc.org/en/master/development/tutorials/autodoc_ext.html#autodoc-ext-tutorial

[jenshnielsen]

For reference this is a libcst transform that can perform this rewrite at least in a simple example

#%%
import libcst as cst
from libcst.codemod import CodemodContext, VisitorBasedCodemodCommand


class AddParameterTransformer(VisitorBasedCodemodCommand):
    def leave_SimpleStatementLine(self, original_node: cst.SimpleStatementLine, updated_node: cst.SimpleStatementLine) -> cst.SimpleStatementLine:
        if isinstance(updated_node.body[0], cst.Expr):
            call_node  = updated_node.body[0].value
        else:
            return updated_node
        if isinstance(call_node, cst.Call):
            func_node = call_node.func
        else:
            return updated_node

        if isinstance(func_node, cst.Attribute) and func_node.attr.value == "add_parameter":
            arg = call_node.args[0].value
            if isinstance(arg, cst.SimpleString):
                var_name = arg.value.strip('"')
                stm = cst.parse_statement(f"self.{var_name}: Parameter = x")
                new_node = stm.body[0]
                new_node = new_node.with_changes(value=call_node)
                return updated_node.with_changes(body=[new_node])
        return updated_node

def transform_code(code: str) -> str:
    transformer = AddParameterTransformer(CodemodContext())
    module = cst.parse_module(code)
    new_module = module.visit(transformer)
    return new_module.code


# Test the transformation
code = """
self.add_parameter(
    "cool_time",
    label="Cooling Time",
    unit="s",
    get_cmd="PS:CTIME?",
    get_parser=int,
    set_cmd="CONF:PS:CTIME {}",
    vals=Ints(5, 3600),
)
"""

#%%
a = transform_code(code)
print(a)
# %%

import libcst as cst
from libcst.codemod import CodemodContext, VisitorBasedCodemodCommand


class AddParameterTransformer(VisitorBasedCodemodCommand):
    def leave_SimpleStatementLine(self, original_node: cst.SimpleStatementLine, updated_node: cst.SimpleStatementLine) -> cst.SimpleStatementLine:
        if isinstance(updated_node.body[0], cst.Expr):
            call_node  = updated_node.body[0].value
        else:
            return updated_node
        if isinstance(call_node, cst.Call):
            func_node = call_node.func
        else:
            return updated_node

        if isinstance(func_node, cst.Attribute) and func_node.attr.value == "add_parameter":
            arg = call_node.args[0].value
            if isinstance(arg, cst.SimpleString):
                var_name = arg.value.strip('"')

                new_node = cst.AnnAssign(target=cst.Attribute(
                    value=cst.Name(
                        value='self',
                        lpar=[],
                        rpar=[],
                    ),
                    attr=cst.Name(
                        value=var_name,
                        lpar=[],
                        rpar=[],
                    ),
                    dot=cst.Dot(
                        whitespace_before=cst.SimpleWhitespace(
                            value='',
                        ),
                        whitespace_after=cst.SimpleWhitespace(
                            value='',
                        ),
                    ),
                    lpar=[],
                    rpar=[],
                    ),
                    annotation=cst.Annotation(
                        annotation=cst.Name(
                            value='Parameter',
                            lpar=[],
                            rpar=[],
                        ),
                        whitespace_before_indicator=cst.SimpleWhitespace(
                            value='',
                        ),
                        whitespace_after_indicator=cst.SimpleWhitespace(
                            value=' ',
                        ),
                    ),
                    value=call_node
                    )
                return updated_node.with_changes(body=[new_node])
        return updated_node

def transform_code(code: str) -> str:
    transformer = AddParameterTransformer(CodemodContext())
    module = cst.parse_module(code)
    new_module = module.visit(transformer)
    return new_module.code


# Test the transformation
code = """
self.add_parameter(
    "cool_time",
    label="Cooling Time",
    unit="s",
    get_cmd="PS:CTIME?",
    get_parser=int,
    set_cmd="CONF:PS:CTIME {}",
    vals=Ints(5, 3600),
)
"""

#%%
a = transform_code(code)
print(a)