-
Notifications
You must be signed in to change notification settings - Fork 100
Adds _extended_purview attribute to Causal Links + pretty formatting of extended_purview #23
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
Conversation
pyphi/actual.py
Outdated
| def is_not_superset(purview): | ||
| return np.all([(not set(purview).issuperset(set(purview2))) or | ||
| (set(purview) == set(purview2)) for purview2 in purviews]) | ||
|
|
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.
Will, is this kind of inner function declaration, ok?
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, that's a typical pattern when using filter. However, sometimes it makes sense to refactor the function into a general-purpose function that's available elsewhere. In this case, we can just leave it in here.
You don't need to use np.all here, since the argument is not already a NumPy array. You can just use the built-in all, which will be more efficient, since the argument doesn't need to be converted into a NumPy array first.
Finally, the logic here is overcomplicated. Your function is equivalent to the following:
def is_minimal(purview):
return all(set(purview).issubset(other_purview) for other_purview in purviews)Note that I'm not constructing a list here, but rather a generator expression (there are no square brackets around the comprehension inside all()). This is because we only need to look at each element in the sequence once, and in this type of situation it's generally more efficient to use generators.
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.
Fixed, except from the logic part becauseis_minimal is not equivalent to is_not_superset, e.g. purview = (1,2), purviews = [(1,2,3), (1,2), (0)], is_not_superset(purview) returns True, while is_minimal(purview) returns False.
pyphi/actual.py
Outdated
| def is_not_superset(purview): | ||
| return np.all([(not set(purview).issuperset(set(purview2))) or | ||
| (set(purview) == set(purview2)) for purview2 in purviews]) | ||
|
|
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, that's a typical pattern when using filter. However, sometimes it makes sense to refactor the function into a general-purpose function that's available elsewhere. In this case, we can just leave it in here.
You don't need to use np.all here, since the argument is not already a NumPy array. You can just use the built-in all, which will be more efficient, since the argument doesn't need to be converted into a NumPy array first.
Finally, the logic here is overcomplicated. Your function is equivalent to the following:
def is_minimal(purview):
return all(set(purview).issubset(other_purview) for other_purview in purviews)Note that I'm not constructing a list here, but rather a generator expression (there are no square brackets around the comprehension inside all()). This is because we only need to look at each element in the sequence once, and in this type of situation it's generally more efficient to use generators.
pyphi/models/actual_causation.py
Outdated
| def __init__(self, ria): | ||
| def __init__(self, ria, extended_purview=None): | ||
| self._ria = ria | ||
| self._extended_purview = extended_purview |
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 would cast this to a tuple:
self._extended_purview = tuple(extended_purview)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.
if extended_purview is None wouldn't this cast lead to an type error?
Maybe something like self._extended_purview = tuple(extended_purview) if extended_purview is not None else None?
|
@renzocom sup |
SUP WILL |
|
Nice work Renzo🥇 |
In actual causation, when computing causal links, it often occurs that purviews have the same alpha. Currently, the
find_causal_link()method returns one arbitrary purview from the ones with maximum alpha.The
find_causal_link()method now adds a hidden attribute_extended_purviewto the returnedCausalLinkwith all the purviews with equivalent maximum alpha which are not supersets of each other. For example, if purviews (A,B), (B,C), (A,B,D) have the same maximum alpha, the extended purview is ((A,B),(B,C)).I've also added pretty formatting methods to print the CausalLinks when they have extended purviews so they look like:
α = 0.415 [[S1, A], [A, D]] ◀━━ [M1]