add add_entry and anti_restrict methods to SkewTableau #39918
Conversation
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/sage/combinat/tableau.py:3193
- The new condition in add_entry adds a constraint that may unintentionally block valid cells in edge cases. Please verify that this additional check correctly handles all tableau shapes and add a comment explaining its intent.
if c == len(tab_r) and (r == 0 or len(tab_r) < len(tab[r-1])):
| sage: S.anti_restrict(1).anti_restrict(2) == S.anti_restrict(2) | ||
| True | ||
| """ | ||
| t_new = [[None if g <= n else g for g in row] for row in self] |
There was a problem hiding this comment.
Comparing elements to n without first checking for None may raise a TypeError if a row contains None values. Consider adding an explicit check (e.g., 'if g is not None and g <= n') to safely perform the comparison.
| t_new = [[None if g <= n else g for g in row] for row in self] | |
| t_new = [[None if g is not None and g <= n else g for g in row] for row in self] |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Write a test to address this.
There was a problem hiding this comment.
Just kidding - None acts like -oo
| sage: S.add_entry([1000,1000], 3) | ||
| Traceback (most recent call last): | ||
| ... | ||
| IndexError: (1000, 1000) is not an addable cell of the tableau |
There was a problem hiding this comment.
Maybe add tests for [0,1000], [1000,0] also maybe adding [1000,0] to [2,1]/[1] (should err). This should address any concerns Copilot is potentially raising.
| if c == len(tab_r) and (r == 0 or len(tab_r) < len(tab[r-1])): | ||
| tab_r.append(m) | ||
| else: | ||
| raise IndexError('%s is not an addable cell of the tableau' % ((r, c),)) |
There was a problem hiding this comment.
Consider saying if the issue is with row or column?
| if r == len(tab) and c == 0: | ||
| tab.append([m]) | ||
| else: | ||
| raise IndexError('%s is not an addable cell of the tableau' % ((r, c),)) |
There was a problem hiding this comment.
Maybe check here for bottom left cell None (per our in-person conversation)
| else: | ||
| try: | ||
| return self.parent().Element(tab) | ||
| except Exception: |
There was a problem hiding this comment.
Should this except be more narrow? (Maybe also change in regular Tableau?)
| @@ -3190,7 +3190,7 @@ def add_entry(self, cell, m): | |||
| raise IndexError('%s is not an addable cell of the tableau' % ((r, c),)) | |||
| else: | |||
| tab_r = tab[r] | |||
| if c == len(tab_r): | |||
| if c == len(tab_r) and (r == 0 or len(tab_r) < len(tab[r-1])): | |||
There was a problem hiding this comment.
Add a test where c== len(tab_r) and r != 0, and another test where c == len(tab_r) and len(tab_r) < len(tab[r-1]).
add_entryandanti_restrictare already existing methods inTableau, but they also make sense forSkewTableau.This PR ports these methods to
SkewTableau, and adds a simple check toadd_entryto avoid trying to construct a new tableau when the given cell is too large.📝 Checklist
⌛ Dependencies
None