IMP: recognize type-mapped Bool variables as Bool variables#401
IMP: recognize type-mapped Bool variables as Bool variables#401colinvwood wants to merge 1 commit into
Bool variables as Bool variables#401Conversation
|
Needed by qiime2/q2-dada2#190 |
ebolyen
left a comment
ebolyen
left a comment
There was a problem hiding this comment.
I don't think you actually need this PR for what you wanted to do, but I have no objection to making it so that TypeVar where all branches are Bool is treated the same as Bool % TypeVar.
| def _is_bool_ast(ast): | ||
| if ast['type'] == 'expression': | ||
| return ast['name'] == 'Bool' | ||
| if ast['type'] == 'variable': |
There was a problem hiding this comment.
This branch isn't actually tested I think because bool-flag-swaps-output-method uses:
Bool % (Choices(True)¹ | Choices(False)²)
instead of
Bool % Choices(True)¹ | Bool % Choices(False)²
Which means that the name of the first type is still just "Bool" which worked before I think. This maybe also suggests that the DADA2 PR can just lift the annotation up to the predicate instead and q2cli will do what you want.
| return multiple, is_bool_flag, metadata | ||
|
|
||
|
|
||
| def _is_bool_ast(ast): |
There was a problem hiding this comment.
I would drop the ast bit and call to_ast() inside the method.
| return ast['name'] == 'Bool' | ||
| if ast['type'] == 'variable': | ||
| index = ast['index'] | ||
| return all(_is_bool_ast(row[index]) for row in ast['mapping']) |
There was a problem hiding this comment.
This makes sense, and I think the AST is actually the easier way to work this out. I checked if it would be reasonable to just use the type's own fields, but they are annoying in this case.
There was a problem hiding this comment.
Just kidding, we can do this instead:
def _is_bool(type_expr):
return all(x.name == 'Bool' for x in type_expr) and not type_expr.is_bottom()__iter__ is a fancy way to get project Union/TypeVars into iterables (I think this makes it a kind of homomorphism over the two monoids) which work on base types as well as higher-level constructs:
In [5]: T
Out[5]: Bool % Choices(False)¹ | Bool % Choices(True)²
In [6]: list(T)
Out[6]: [Bool % Choices(False), Bool % Choices(True)]
In [7]: list(Bool)
Out[7]: [Bool]The is_bottom() check avoids all([]) from being trivially boolean.
|
What's your preference re keeping this? Indeed your changes in |
|
I think there's no harm in making this work. I guess the only difficulty is testing and if it's worth adding another action to dummy_plugin for it. I'm also fine with putting it on the back-burner. There are perhaps other ways to handle this directly, such as distributing the TypeVar branches over the predicate so that it just always normalizes. Although it may be tricky to ensure that it still looks correct at different call-sites. |
3 participants
Description
Fixes an issue where type-mapped
Boolvariables were not recognized asBools and rendering on the cli fell back to the standard--p-param VALUEformat.AI Disclosure
AI Usage Details
Generated by Codex while working on qiime2/q2-dada2#190