fix(pumpkin-core): Creation and Insertion in Sparse Set

[maartenflippo]

No description provided.

[EmirDe]

At face value, this looks like a nice addition. Why is this a draft PR?

[ImkoMarijnissen]

I think the setup of the test case is incorrect; the SparseSet assumes that the indices map to the elements in the provided array, but you are providing a function which just uses the number as the index.

The following test case would pass (and would test the behaviour that you wanted):

#[test]
fn remove_temporarily() {
    let mut sparse_set = SparseSet::new(vec![0, 1, 2], mapping_function);
    sparse_set.remove_temporarily(&2);
    assert!(!sparse_set.contains(&2));

    sparse_set.remove_temporarily(&1);
    sparse_set.remove_temporarily(&0);
    assert!(sparse_set.is_empty());
}

[ImkoMarijnissen]

I have fixed a bug in the insertion method of sparse set which could explain some of the behaviour that you saw with the Random variable selector.

[maartenflippo]

Our Random variable selector can enter the infinite loop. If it is used based on a minizinc search annotation, the setup looks like the test case I added that fails. See the code below:

fn select_variable(&mut self, context: &mut SelectionContext) -> Option<DomainId> {
      if self.variables.is_empty() {
          return None;
      }

      let mut variable = *self.variables.get(
          context
              .random()
              .generate_usize_in_range(0..self.variables.len()),
      );

      while context.is_integer_fixed(variable) {
          self.variables.remove_temporarily(&variable);
          if self.variables.is_empty() {
              return None;
          }

          variable = *self.variables.get(
              context
                  .random()
                  .generate_usize_in_range(0..self.variables.len()),
          );
      }

      Some(variable)
  }

Nowhere does it check any invariants for the sparse set, it is created from an iterator over DomainIds.

I also think that the paper we cite does care about how the structure is initialized. Given any sparse set of numbers it works. The mapping is created inside the data structure. Perhaps that is the better approach for us as well?

[ImkoMarijnissen]

Our Random variable selector can enter the infinite loop. If it is used based on a minizinc search annotation, the setup looks like the test case I added that fails. [...]

I think that that test case should not occur using the MiniZinc search annotations since the backup strategy should contain all variables in the problem, meaning that there should be no indexing issues (which is what happens in the example). Could you provide the instance on which we enter an infinite loop?

The paper mentions the following:

The domD array contains all values in the range [1, N]. [...]. The mapD array maps values to their position in the domD array.

In the above representation, we use an array to implement mapD structure. For a very sparse initial domain this is not optimal since it requires a space proportional to the difference between the largest and smallest value. We can imagine using another implementation of mapD structure, for example with Hashmaps also allowing fast access based on keys consuming a memory proportional to the number of initial values in the domain.

We indeed do not do this in the initialisation step since we assume that the user will provide us with a mapping function which maps each value in the domain to an index in the indices struct (i.e., each element in domain should be mapped (via a bijective function) to an element in [0, |domain|]). As you rightly point out, this is indeed not checked, I will add a check for this!

I also think that the paper we cite does care about how the structure is initialized. Given any sparse set of numbers it works. The mapping is created inside the data structure. Perhaps that is the better approach for us as well?

This is a matter of preference, I think that we could provide two methods: new and new_unchecked, the former creates the array (with potential space overhead if the indices provided is not contiguous) based on the provided indices, while the other assumes that you provide a bijective function. Does that seem reasonable?

[maartenflippo]

This is a matter of preference, I think that we could provide two methods: new and new_unchecked, the former creates the array (with potential space overhead if the indices provided is not contiguous) based on the provided indices, while the other assumes that you provide a bijective function. Does that seem reasonable?

The reason I am so confused by this is that everywhere we create a mapping it looks the same (save for type conversions between different integer types). Given that I am already confused by this, I can only imagine how an external user will feel, which is something I consider important given this is in the public API of our crate.

However, based on the new test cases the behavior of the set, once created, is at least more predictable so I am okay with leaving the discussion as is.

[EmirDe]

I am very curious about this. Let us discuss in person!

[maartenflippo]

The usage of StorageKey is great here!