Prevent order mismatch in words function#41197
Conversation
cxzhong
changed the title
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/sage/combinat/words/morphism.py:893
- This method raises ValueError - should raise an ArithmeticError or return NotImplemented instead.
This method raises ValueError - should raise an ArithmeticError or return NotImplemented instead.
This method raises ValueError - should raise an ArithmeticError or return NotImplemented instead.
def __mul__(self, other):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cxzhong
changed the title
|
We should put the _is_alphabet_included_with_order logic in this or the alphabet file? |
cxzhong
requested review from
dcoudert,
fchapoton,
mantepse and
vincentmacri
Updated error handling for morphism composition to provide clearer messages regarding codomain and domain compatibility.
Refactor composition validation logic in morphism.py.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Clarify error message for morphism order mismatch.
Updated error messages to provide clearer feedback when the codomain alphabet is not included in the domain alphabet with the correct ordering.
|
Documentation preview for this PR (built with commit 6680870; changes) is ready! 🎉 |
|
@videlec Can you help me review this? |
|
It feels like this logic should be higher up in the category hierarchy. Namely, it sounds like you are checking whether a given poset (which here is totally ordered) is a sub-poset of another one. There could be a method in Another argument to think about a more generic place to have such implementation is that specialized classes could implement faster checks. One very common use case is an alphabet which is a subset of either roman letters or integers (with the induced orders). |
| source_list = list(source_alphabet) | ||
| target_list = list(target_alphabet) | ||
|
|
||
| if not all(a in target_alphabet for a in source_list): |
There was a problem hiding this comment.
this test would be much faster if target_alphabet was a set.
Since you also need the positions in the alphabet, you should directly define the mapping target_positions and use it here instead of target_alphabet.
| source_positions = [target_positions[letter] for letter in source_list] | ||
|
|
||
| # Check if positions are in increasing order | ||
| return all(source_positions[i] < source_positions[i+1] |
There was a problem hiding this comment.
could also be return all(p < q for p, q in zip(source_positions, source_positions[1:]))
There was a problem hiding this comment.
That would be less efficient though as source_positions[1:] makes a copy.
There was a problem hiding this comment.
Right.
Another proposal to check that source_alphabet is a subset of target_alphabet and that the elements in source_alphabet appear in the same order in target_alphabet is:
targets = list(target_alphabet)
n_targets = len(targets)
idx = 0
for a in source_alphabet:
while idx < n_targets and targets[idx] != a:
idx += 1
if idx == n_targets:
return False
return True| def is_self_composable(self): | ||
| r""" | ||
| Return whether the codomain of ``self`` is contained in the domain. | ||
| Return whether the codomain of ``self`` is contained in the domain |
|
|
||
| def _is_alphabet_included_with_order(self, source_alphabet, target_alphabet): | ||
| """ | ||
| Check if ``source_alphabet`` is included in ``target_alphabet`` with the correct ordering. |
There was a problem hiding this comment.
ensure alignment on 80 columns.
| Traceback (most recent call last): | ||
| ... | ||
| KeyError: 'b' | ||
| ValueError: the codomain alphabet of the second morphism must be included in the domain alphabet of the first morphism with the same ordering |
There was a problem hiding this comment.
you can align on 80 columns
|
@videlec, let us know if you prefer another solution. |
I think this is not the long-term way to solve the issue. However it does improve the situation! I definitely agree with the positive review. |
sagemathgh-41197: Prevent order mismatch in words function Updated error handling for morphism composition to provide clearer messages regarding codomain and domain compatibility. Fix sagemath#40391 I think it may be strict in mathematical definition. But it is useful to prevent order mismatch. to get some error results in this issue. And for ``Words("ab")``, we think ``b>a``, for ``Words("ba")``, we think ``a>b``, so they are different order in sage's words system. We prevent words order mismatch behavior to output some mathematical incorrect output <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#41197 Reported by: Chenxin Zhong Reviewer(s): Copilot, David Coudert, Vincent Delecroix
Updated error handling for morphism composition to provide clearer messages regarding codomain and domain compatibility.
Fix #40391
I think it may be strict in mathematical definition. But it is useful to prevent order mismatch. to get some error results in this issue.
And for
Words("ab"), we thinkb>a, forWords("ba"), we thinka>b, so they are different order in sage's words system.We prevent words order mismatch behavior to output some mathematical incorrect output
📝 Checklist
⌛ Dependencies