Adding function Subsets to the mathics #685
Conversation
sn6uv
left a comment
sn6uv
left a comment
There was a problem hiding this comment.
Looks good. Just a couple of edge cases and cleanups.
mathics/builtin/combinatorial.py
Outdated
| if not n.get_head_name() == 'System`Integer' or n.to_python() < 0 : | ||
| return evaluation.message('Subsets', 'nninfseq', expr) | ||
|
|
||
| tlen = int(n.to_python()) |
There was a problem hiding this comment.
we have a utility method for this, n.get_int_value() which returns an int or None.
mathics/builtin/combinatorial.py
Outdated
|
|
||
| tlen = int(n.to_python()) | ||
|
|
||
| nested_list = [Expression('List', *c) for i in range(0, tlen + 1) for c in combinations(tlist, i)] |
There was a problem hiding this comment.
range(0, tlen +1) is range(tlen+1)
mathics/builtin/combinatorial.py
Outdated
|
|
||
| if n_len == 1: | ||
| elem1 = n_python[0] | ||
| if not n.leaves[0].get_head_name() == "System`Integer" or elem1 < 0 : |
There was a problem hiding this comment.
Not all values can be converted to_python so elem1 may be None here. Better to use get_int_value() here too.
mathics/builtin/combinatorial.py
Outdated
| if n_len == 2: | ||
| elem1 = n_python[0] | ||
| elem2 = n_python[1] | ||
| if not n.leaves[0].get_head_name() == "System`Integer" or not n.leaves[1].get_head_name() == "System`Integer" or elem1 < 0 or elem2 < 0 : |
mathics/builtin/combinatorial.py
Outdated
| step_n = 1 | ||
|
|
||
| if n_len == 3: | ||
| if not n.leaves[0].get_head_name() == "System`Integer" or not n.leaves[1].get_head_name() == "System`Integer" or not n.leaves[2].get_head_name() == "System`Integer" : |
mathics/builtin/combinatorial.py
Outdated
| tlist = [x for x in list.leaves] | ||
| expr = Expression('Subsets', list, n) | ||
|
|
||
| if not n.get_head_name() == 'System`Integer' or n.to_python() < 0 : |
|
Thanks for your feedback @sn6uv . I've just pushed the latest commit for adding Subsets[] with some changes as your recommendations. Please take a look and give me some feedback if any. Thanks in advance! |
mathics/builtin/combinatorial.py
Outdated
| return evaluation.message('Subsets', 'nninfseq', expr) | ||
|
|
||
| tlen = int(n.to_python()) | ||
| nested_list = [Expression('List', *c) for i in range(n_value + 1) for c in combinations([x for x in list.leaves], i)] |
There was a problem hiding this comment.
... for c in combinations(list.leaves, ....?
mathics/builtin/combinatorial.py
Outdated
|
|
||
| n_python = n.to_python() | ||
| n_len = len(n_python) | ||
| n_len = n.leaves.__len__() |
mathics/builtin/combinatorial.py
Outdated
| elem1 = n_python[0] | ||
| if not n.leaves[0].get_head_name() == "System`Integer" or elem1 < 0 : | ||
| elem1 = n.leaves[0].get_int_value() | ||
| if not elem1 or elem1 < 0 : |
There was a problem hiding this comment.
not elem1 will also be true if elem1 is 0, use elem1 is not None
mathics/builtin/combinatorial.py
Outdated
| return evaluation.message('Subsets', 'nninfseq', expr) | ||
|
|
||
| nested_list = [Expression('List', *c) for i in range(min_n, max_n, step_n) for c in combinations(tlist, i)] | ||
| nested_list = [Expression('List', *c) for i in range(min_n, max_n, step_n) for c in combinations([x for x in list.leaves], i)] |
There was a problem hiding this comment.
combinations(list.leaves... here too
mathics/builtin/combinatorial.py
Outdated
| elem1 = n.leaves[0].get_int_value() | ||
| elem2 = n.leaves[1].get_int_value() | ||
| elem3 = n.leaves[2].get_int_value() | ||
| if elem1 is None or elem2 is None or elem3 is None : |
There was a problem hiding this comment.
Should we also be checking that elem1 and elem2 aren't negative?
| max_n = elem2 + 1 | ||
| step_n = 1 | ||
|
|
||
| elif n_len == 3: |
|
Thanks for your feedback @sn6uv . I've just pushed the latest source code with the following changes:
|
|
I am not seeing anything left unresolved. What am I missing? |
5 participants
Adding Subsets[]