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

[WIP] Add convenience methods to DelegateParameter #6832

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

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Jan 27, 2025

DelegateParameter now includes validators of its source Parameter into its validators. This ensures that a DelegateParameter
with a non numeric source parameter is registered correctly in a measurement when the DelegateParameter it self does not
set a validator. Furthermore `DelegateParameterhas gained aroot_source`` attribute that makes it easier to get the
root source of a ``DelegateParameter`` that delegates to another ``DelegateParameter``.

Alternative to #6817

@jenshnielsen jenshnielsen requested a review from a team as a code owner January 27, 2025 12:21
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.39%. Comparing base (83d4cd5) to head (d3d58bf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6832      +/-   ##
==========================================
+ Coverage   69.37%   69.39%   +0.01%     
==========================================
  Files         341      341              
  Lines       31381    31399      +18     
==========================================
+ Hits        21771    21789      +18     
  Misses       9610     9610              

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

@jenshnielsen jenshnielsen changed the title [wip] extend test_value_validation to cover more cases [wip] extend delegate parameter validators with validators from source parameter. Jan 27, 2025
@jenshnielsen jenshnielsen changed the title [wip] extend delegate parameter validators with validators from source parameter. Extend DelegateParameter validators with validators from source parameter and add root_source property Feb 3, 2025
meas.register_parameter(delegate_param, setpoints=(x_param,))
assert len(meas.parameters) == 2
assert meas.parameters["delegate"].type == "complex"
assert meas.parameters["x"].type == "numeric"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lets also add a test for delegate of delegate of complex

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A -> B -> C -> None

Add source to C

verify that A has the correct validator.

delegate_param.source.vals = None

assert delegate_param.validators == (some_other_validator,)
assert delegate_param.vals == some_other_validator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extend to 2 delegate parameters too

@jenshnielsen
Copy link
Collaborator Author

Moved the validators to #6865 will figure out how to integrate the test with the infer module here in a more consistent way

@jenshnielsen jenshnielsen changed the title Extend DelegateParameter validators with validators from source parameter and add root_source property [WIP] Add convenience methods to DelegateParameter Feb 7, 2025
@@ -0,0 +1,4 @@
``DelegateParameter`` now includes validators of its source Parameter into its validators. This ensures that a ``DelegateParameter``
with a non numeric source parameter is registered correctly in a measurement when the ``DelegateParameter`` it self does not
set a validator. Furthermore `DelegateParameter`` has gained ``root_source`` and ``root_delegate`` attributes that makes it easier to get the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set a validator. Furthermore `DelegateParameter`` has gained ``root_source`` and ``root_delegate`` attributes that makes it easier to get the
set a validator. Furthermore ``DelegateParameter`` has gained ``root_source`` and ``root_delegate`` attributes that makes it easier to get the

"""
The root source parameter that this :class:`DelegateParameter` is bound to
or ``None`` if this :class:`DelegateParameter` is unbound. If
the source is it self a DelegateParameter it will recursively return that Parameter's
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the source is it self a DelegateParameter it will recursively return that Parameter's
the source is it self a :class:`DelegateParameter`, this property will recursively return that Parameter's

The root source parameter that this :class:`DelegateParameter` is bound to
or ``None`` if this :class:`DelegateParameter` is unbound. If
the source is it self a DelegateParameter it will recursively return that Parameter's
source until a non DelegateParameter is found. For a non DelegateParameter source
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
source until a non DelegateParameter is found. For a non DelegateParameter source
source until a non DelegateParameter is found. If a source of this parameter is not a DelegateParameter,

source until a non DelegateParameter is found. For a non DelegateParameter source
this behaves the same as ``self.source``

:getter: Returns the current source.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:getter: Returns the current source.
:getter: Returns the current source parameter that's at the end of a chain of DelegateParameters.

@property
def root_delegate(self) -> DelegateParameter:
"""
If this parameter is part of a chain of DelegateParameters return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If this parameter is part of a chain of DelegateParameters return
If this parameter is part of a chain of DelegateParameters, return

def root_delegate(self) -> DelegateParameter:
"""
If this parameter is part of a chain of DelegateParameters return
the first Parameter in the chain that has a non DelegateParameter source
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the first Parameter in the chain that has a non DelegateParameter source
the first Parameter in the chain that has a non DelegateParameter source,

self.source.validators if self.source is not None else ()
)

return tuple(self._vals) + source_validators
Copy link
Contributor

Choose a reason for hiding this comment

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

just wonder, if it's cleaner (and even possible) to use super here?

Suggested change
return tuple(self._vals) + source_validators
return super().validators + source_validators

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.

3 participants