Skip to content

Conversation

@Hapsa21
Copy link
Contributor

@Hapsa21 Hapsa21 commented Nov 19, 2025

Description

This PR fixes a fragile doctest failure reported in #41193.

The Issue:
The failing test in src/sage/rings/polynomial/multi_polynomial_ideal.py used sorted(..., key=str) on a list of dictionaries. In Python, the string representation of a dictionary depends on the insertion order of its keys. Since the upstream library msolve likely changed its internal insertion order in newer versions, key=str produced a different sort order than expected, causing the test to fail despite the mathematical results being correct.

The Fix:
I updated the test command to replace the unstable sort key with a robust one. This sorts the dictionary items before stringifying, ensuring the sorting relies on the dictionary contents rather than their internal memory layout.

# Old (fragile)
key=str

# New (robust)
key=lambda d: str(sorted(d.items()))

Verification
I verified that this change makes the sorting immune to dictionary insertion order.

Proof of Fix: Running the following script demonstrates why key=str failed and why the new key succeeds:

dict_A = {'x': 100, 'y': 500}
dict_B = {'y': 500, 'x': 100}

#  ---Old Behavior (key=str)--- 
# str(dict_A) is "{'x': 100, 'y': 500}"
# str(dict_B) is "{'y': 500, 'x': 100}"
print(f"Old method equal? {str(dict_A) == str(dict_B)}")
# Result: False (This caused the test failure)

# --- New Behavior (The Fix) ---
my_key = lambda d: str(sorted(d.items()))
print(f"New method equal? {my_key(dict_A) == my_key(dict_B)}")
# Result: True (The sort is now stable)

This ensures the doctest passes reliably regardless of the upstream library's behavior.

Issue

Fixes #41193 Relates to #41192

📝 Checklist

  • 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

None.

@cxzhong
Copy link
Contributor

cxzhong commented Nov 20, 2025

Thank you. LGTM

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 21, 2025
sagemathgh-41206: Fix fragile doctest sorting in multi_polynomial_ideal
    
### Description
This PR fixes a fragile doctest failure reported in sagemath#41193.

**The Issue:**
The failing test in
`src/sage/rings/polynomial/multi_polynomial_ideal.py` used `sorted(...,
key=str)` on a list of dictionaries. In Python, the string
representation of a dictionary depends on the insertion order of its
keys. Since the upstream library `msolve` likely changed its internal
insertion order in newer versions, `key=str` produced a different sort
order than expected, causing the test to fail despite the mathematical
results being correct.

**The Fix:**
I updated the test command to replace the unstable sort key with a
robust one. This sorts the dictionary items before stringifying,
ensuring the sorting relies on the dictionary *contents* rather than
their internal memory layout.

```python
# Old (fragile)
key=str

# New (robust)
key=lambda d: str(sorted(d.items()))
```
**Verification**
I verified that this change makes the sorting immune to dictionary
insertion order.

**Proof of Fix**: Running the following script demonstrates why
`key=str` failed and why the new key succeeds:
```python

dict_A = {'x': 100, 'y': 500}
dict_B = {'y': 500, 'x': 100}

#  ---Old Behavior (key=str)---
# str(dict_A) is "{'x': 100, 'y': 500}"
# str(dict_B) is "{'y': 500, 'x': 100}"
print(f"Old method equal? {str(dict_A) == str(dict_B)}")
# Result: False (This caused the test failure)

# --- New Behavior (The Fix) ---
my_key = lambda d: str(sorted(d.items()))
print(f"New method equal? {my_key(dict_A) == my_key(dict_B)}")
# Result: True (The sort is now stable)
```
This ensures the doctest passes reliably regardless of the upstream
library's behavior.

### **Issue**
Fixes sagemath#41193 Relates to sagemath#41192

### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

None.
    
URL: sagemath#41206
Reported by: Hetarth Jodha
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A doctest in multi_polynomial_ideal.py fails to recognize a synonym of the expected answer

2 participants