-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add a helper function for looking at variables #170
Conversation
Click here to view all benchmarks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing with @hombit, we think we do not really need the collapse
feature, since it's hard to define how close is close enough to be considered the same. I think we can keep just the collapse=False
behavior.
In terms of "close enough", I had been thinking effectively an exact match. If the value is exactly the same then it is likely just information copied between nodes. But if we don't need this, then it is easier not to have it. If we do not need |
Looks good to me. I think we do not need |
@mi-dai I changed the name from I also re-added the functionality to pass a single string or pass parameter names as a positional argument. I don't think we should include the full name option though, because we can already access that with the |
assert results["a.v1"] == 1.0 | ||
assert results["a.v2"] == 2.0 | ||
assert results["c.v2"] == 5.0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking something like extract_parameters(['a.v1','c.v2','v3'])
which will return {'a.v1: 1, 'c.v2': 5, 'a.v3': 3, 'c.v3': 3, 'e.v3': 3}. Or extract_parameters(['a.v1','v5'])
which will return {'a.v1': 1, 'v5': 7} or {'a.v1': 1, 'e.v5': 7} (either is fine but maybe the former is slightly preferred).
For example, I wanted to get a quick plot of the simulated Hubble Diagram using host redshift, so I'd want to do extract_parameters(['host.redshift','x0','x1','c'])
, and I don't care (or don't know) what node x0
,x1
or c
is from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That behavior was the point of the collapse
logic. It allowed returning a single value for a parameter using just the parameter name.
In your case of extract_parameters(['host.redshift','x0','x1','c'])
how do you want to handle duplicates? If we find instances of x0
in two different nodes we could:
- Throw an error [initial behavior]
- Collapse them if they are identical
2a. Throw an error if they are not identical
2b. Return both in expanded form if they are not identical [second round behavior] - Return all values [current behavior]
- Return an arbitrary one of the
x0
values. [Not recommended as the behavior becomes undefined].
More concretely if someone where to call extract_parameters(['ra', 'dec', 't0'])
where the host and source have two different values of ra
, how would you like the code to behave?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to output duplicate parameter names with their node names (say a.x0
, b.x0
), regardless of whether they are identical in values.
If someone calls extract_parameters(['ra','dec','t0'])
, we'll return {'host.ra','source.ra','anything_else.ra', ...}, even some of them may be identical in their values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to add that I think extract_parameters(node=[],parameters=[])
is still useful. Maybe parameters can be expended to include extract_parameters(parameters=['host.redshift','x0','x1','c'])
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've got it, but let me check.
In the case of a state:
a.v1 -> 1.0
a.v2 -> 2.0
a.v3 -> 3.0
b.v1 -> 4.0
b.v2 -> 5.0
b.v4 -> 6.0
c.v1 -> 7.0
and the query extract_parameters(parameters=['a.v1', 'v2', 'v3', 'b.v4'])
you would want the function to return the dictionary:
{
"a.v1": 1.0,
"a.v2": 2.0,
"b.v2": 5.0,
"v3": 3.0,
"b.v4": 6.0,
}
Specifically:
- The full name are always preserved (even if the parameter does not exist in other nodes).
- The shortened name is preserved if the parameter is unique.
- The shortened name is transformed into multiple long names if the parameter is not unique.
Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented those rules. I've dropped the nodes
parameter for now, because it is not clear how to incorporate that as well. We can always extract all parameters with to_dict
.
For common parameters, we might want to extract them from the GraphState without going through the node. This is useful in both logging and debugging. This PR adds a helper function that can extract (uniquely named) parameters from arbitrary nodes in the graph.