RangeSet next method fails

[GiorgioBalestrieri]

Hi,

The next method of a RangeSet fails consistently.
It seems the reason is that RangeSet is always virtual (even if I pass virtual=False to the constructor), and virtual sets do not populate the ordered_dict field (or better, keep it as an empty dictionary).

I know by definition + 1 should be a reasonable replacement for next within a RangeSet, what I don't like is the fact that I need to change my constraint definitions if I change from a Set to a RangeSet, which is not great (and in particular breaks the Liskov substitution principle of SOLID).

This extends to any Set which is both ordered and virtual.

The easiest solution I see is to override the next methods for RangeSets as

def next(self, match_element, k=1):
    return self[match_element + k]

An alternative is to allow the creation of non-virtual RangeSets., if virtual=False is passed to the constructor.

[jsiirola]

There are a number of known issues with Set/RangeSet in Pyomo (see #326), and a complete redesign of the Set / RangeSet implementation is underway (see set-rewrite on jsiirola/pyomo). This issue has already been addressed on that branch (although it is still very much a work-in-progress, and is NOT yet suitable for use by anyone).

[GiorgioBalestrieri]

This is truly great, the pace of improvement of Pyomo is impressive.

Depending on how long you expect it to take to merge the set-rewrite branch, I would be happy to open a pull request with the override for next proposed above, it seems a very quick fix and I don't see any potential drawbacks, except fixing only a subset of the problem.

[jsiirola]

PRs are always welcome. I can tell you that you will want to expand on your proposed solution: RangeSet admits steps other than 1, and starting points other than 1, so you would need to take that into account. I suspect that you can adopt something similar to the implementation of next in Set.

I am hoping that I will have the rewrite "finished" in the next couple weeks. The rewrite is currently written "alongside" the existing Set module, so my integration plan is to go through two PR review cycles: the first PR will include just the new system, without integrating it with the rest of Pyomo. This is mostly to complete a design review of the new system before going through the effort of integrating it with Pyomo and fixing any inadvertent backwards incompatibilities. The second PR will be the integration. If all goes well, the whole process will be done by the end of the year; but, "it's tough to make predictions, especially about the future."

[GiorgioBalestrieri]

Ok, I laughed.

Point taken about taking into account _start and _step. Something along these lines should work:

def next(self, match_element, k=1):
    if match_element < self._start:
        raise ValueError(f"match_element ({match_element}) must be larger than starting element of RangeSet ({self._start}).")
        
    if (match_element - self._start) % self._step != 0:
        raise ValueError(f"match_element {match_element} is not valid for RangeSet({self._start}, {self._end}, {self._step})")
    
    i = match_element + k * self._step
    
    if i > self._end:
        raise ValueError(f"match_element {match_element} is too large for RangeSet with end {self._end}.")
    
    return i

I'll do something more structured and create a PR by end of the week.