Macaulay2 / Macaulay2/M2

An empty monomial ordering block is dropped inside the engine, so selectInSubring counts blocks wrongly and silently returns the wrong columns

Open
#4,621 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bugs directory Engine
Dominant language
Macaulay2
Stars
435
Forks
297
Avg merge
4d 20h
Merged PRs (30d)
11

Description

This issue was triaged from [`bugs/mike/0-empty-monomial-ordering-blocks`](https://github.com/Macaulay2/M2/blob/388c1ff0ce30d83751dea7bc7eac77fdc1305dd7/bugs/mike/0-empty-monomial-ordering-blocks), one of the 857 files removed from the pre-GitHub `bugs/` tree by [`d2c8d27826`](https://github.com/Macaulay2/M2/commit/d2c8d27826) and catalogued in [#36](https://github.com/Macaulay2/M2/issues/36). **The commentary below was written by Claude (Claude Opus 5, via Claude Code)**, not by @d-torrance, whose account posted it -- please weigh it accordingly.

### The original file, verbatim

```text
The answer should be | x y z |, but the Eliminate 0 block is removed during joining.

i1 : R = QQ[x,y,z, MonomialOrder => {Eliminate 0, Eliminate 1}]

o1 = R

o1 : PolynomialRing

i2 : selectInSubring(1,vars R)

o2 = | y z |

1 2
o2 : Matrix R <--- R

My first attempt to fix it was:

Index: monordering.c
===================================================================
--- monordering.c (revision 14174)
+++ monordering.c (working copy)
@@ -332,7 +332,9 @@
case MO_LAURENT_REVLEX:
case MO_REVLEX:
case MO_WEIGHTS:
- return (p->nvars > 0);
+ // it is a bad idea to elide blocks in the ordering, because that changes the number of them, and
+ // functions such as selectInSubring depend on the numbering:
+ // return (p->nvars > 0);
case MO_POSITION_UP:
case MO_POSITION_DOWN:
return 1;

but that broke many tests.

-- Dan

PS:

Here are some useful tests for after fixing the bug:

-- a fix retains null monomial ordering blocks when joining:
R = QQ[x,y,z, MonomialOrder => {Eliminate 0, Eliminate 1}]
assert( (selectInSubring(1,vars R)) === map(R^1,R^{{-1},{-1},{-1}},{{x, y, z}}) )
R = QQ[x,y,z, MonomialOrder => {Eliminate 1, Eliminate 1}]
assert( (selectInSubring(1,vars R)) === map(R^1,R^{{-1},{-1}},{{y, z}}) )
R = QQ[x,y,z, MonomialOrder => {Eliminate 2, Eliminate 1}]
assert( (selectInSubring(1,vars R)) === map(R^1,R^{{-1}},{{z}}) )
```

### Where it stands today

An empty block in a monomial ordering is dropped inside the engine, so `selectInSubring` counts
blocks differently from the way they were written and silently returns the wrong columns.

```m2
i1 : R = QQ[x,y,z, MonomialOrder => {Eliminate 0, Eliminate 1}]

o1 = R

o1 : PolynomialRing

i2 : selectInSubring(1,vars R)

o2 = | y z |

1 2
o2 : Matrix R <-- R
```

`Eliminate 0` eliminates nothing, so the first block imposes no condition and `selectInSubring(1, ...)`
should return all three variables. Instead it returns the answer belonging to the *second* block —
identical to what a ring with only `{Eliminate 1}` gives.

### Where it goes wrong

The top-level monoid keeps the empty block:

```m2
i1 : R = QQ[x,y,z, MonomialOrder => {Eliminate 0, Eliminate 1}];

i2 : (monoid R).Options.MonomialOrder

o2 = {MonomialSize => 32 }
{Weights => {} }
{Weights => {1} }
{GRevLex => {1, 1, 1}}
{Position => Up }
```

So this is not visible from the monoid. But `selectInSubring` does not consult it —
[`matrix2.m2:700-702`](https://github.com/Macaulay2/M2/blob/development/M2/Macaulay2/m2/matrix2.m2#L700-L702)
hands the question straight to the engine as `rawEliminateVariables(i, m.RawMatrix)`, and the engine
filters blocks through `is_good`, which rejects any block covering no variables:

https://github.com/Macaulay2/M2/blob/development/M2/Macaulay2/e/interface/monomial-ordering.cpp#L66-L90

`is_good` gates both the block count and the block iteration (lines 219 and 230, and again at 658 and
669), so the empty `Weights => {}` block is absent from the engine's numbering and every later block
shifts down by one.

### Tests

These come from the bug file, written by Dan. The first fails today; the second and third pass and are
useful as controls, since they differ only in the first block being non-empty:

```m2
R = QQ[x,y,z, MonomialOrder => {Eliminate 0, Eliminate 1}]
assert( (selectInSubring(1,vars R)) === map(R^1,R^{{-1},{-1},{-1}},{{x, y, z}}) )
R = QQ[x,y,z, MonomialOrder => {Eliminate 1, Eliminate 1}]
assert( (selectInSubring(1,vars R)) === map(R^1,R^{{-1},{-1}},{{y, z}}) )
R = QQ[x,y,z, MonomialOrder => {Eliminate 2, Eliminate 1}]
assert( (selectInSubring(1,vars R)) === map(R^1,R^{{-1}},{{z}}) )
```

### A fix that was already tried

The bug file records an attempt at the obvious change — commenting out the `nvars > 0` test so that
empty blocks survive — with the note that it *"broke many tests"*, and the reasoning:

> it is a bad idea to elide blocks in the ordering, because that changes the number of them, and
> functions such as `selectInSubring` depend on the numbering

So the elision is load-bearing somewhere else, and preserving empty blocks wholesale is not the fix.
Reconciling the two numberings, or rejecting an ordering whose blocks cannot be represented, may be
closer.

Not the same as [#883](https://github.com/Macaulay2/M2/issues/883), which is about `selectInSubring`
being under-documented — the term "block" not being defined — and a proposal to replace it with
something clearer. That issue would not be closed by fixing this one.

Adjacent but distinct from [#4593](https://github.com/Macaulay2/M2/issues/4593), which is also about
how `MonomialOrder` blocks are validated: there an over-long weight vector is rejected outside
`MonomialOrder` and silently accepted inside it. Empty blocks appear in that report only as
scaffolding in the examples, and it concerns neither block numbering nor `selectInSubring`. The two
are worth reading together if someone revisits how blocks are checked and stored.

### Where this came from

Cataloguing the `bugs/` directory removed in d2c8d27826 (#36).
`bugs/mike/0-empty-monomial-ordering-blocks` is Dan's write-up, and it opens with the diagnosis this
issue confirms: *"The answer should be `| x y z |`, but the Eliminate 0 block is removed during
joining."*

`open` · disposition `issue` · source of truth: [`bug-triage/catalog.tsv`](https://github.com/d-torrance/M2/blob/bug-triage/bug-triage/catalog.tsv)

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with matrix2.m2 lines 700-702 and the rawEliminateVariables entry point, then inspect is_good and the block-counting iterations in monomial-ordering.cpp. Run the three listed selectInSubring assertions, including the failing Eliminate 0 case; done means empty blocks no longer shift later block numbering without breaking the control cases and existing tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.