Add some traits to the rust version

[datdenkikniet]

Add some traits that lets us more properly define what data we take in, and what data we produce out.

Also moves around the enumerate option of the CLI a bit, and puts the CLI code in a subdirectory.

I intend to move hashless, point-list and rotation_reduced over to these traits as well, but fear that the PR will get too big.

I'm pretty bad at making well-sized commits, so squashing this PR is OK.

[datdenkikniet]

@NailLegProcessorDivide if you could take a look at the changes I've made to hashless, I would appreciate it!

I have refactored it to be a bit more rust-y and am fairly sure it's got the same performance, and it is producing the same results. Comments welcome!

Edit: correction, it seems that the changes I made made the implementation ever so slightly slower, but I'm not sure why that happens.

[NailLegProcessorDivide]

I havent checked the code yet but should have time later tonight. the performance seems virtually identical however which is good.

I have an idea for a way to use a more advanced polycube rep that caches some of the continuity checks that are still a huge amount of run time but I think it would be best to try to implement it against your traits any way.

My attempt to use a couple of AVX2 intrinsics to speed up continuity were 4x slower so as long as its within a percent or 2 it isnt that big of a deal imo.

[NailLegProcessorDivide]

I dont know if there would be a performance regression but should cube expansion become a method on the polycube rather than the container.

Im not familiar with the complexity involved for converting expand_cube_map to return an iterator but would you consider that a better option than keeping it part of the solution? this would decouple it from the storage type shich is the only reason that code ends up needing to be copy and pasted from pointlist.rs as well.

Then something like cube.do_cube_expansions().for_each(|new_cube| map.insert_map(new_cube)); instead?

Does this make any sense?

[datdenkikniet]

I dont know if there would be a performance regression but should cube expansion become a method on the polycube rather than the container.
...

Does this make any sense?

Yes, I think it does (make sense)! I feel that it could degrade performance a little (but only a little), as the current implementation is (AFAICT) quite specialized.

However, actually doing the implementation should be trivial, all you need to do is return something that implements Iterator<Item = CubeMapPos<N>> that yields the same items (you basically have to reconstruct any nested for-loops and other state into a struct). So, figuring out if the performance is any different should be relatively easy to discover.

If you want to give it a shot you can look at naive_polycube/expander.rs for inspiration. If not, let me know and I can give it a shot

Edit: not difficult + trivial = not trivial 🤦

[NailLegProcessorDivide]

Yes, I think it does! I feel that it could degrade performance a little as the current implementation is (AFAICT) quite specialized.

However, actually doing the implementation should be trivial, all you need to do is return something that implements Iterator<Item = CubeMapPos<N>> that yields the same items (you basically have to reconstruct any nested for-loops and other state into a struct). If you want to give it a shot you can look at naive_polycube/expander.rs for inspiration.

If not, let me know and I can give it a shot

Edit: not difficult + trivial = not trivial 🤦

I had a brief look at generators which look like they might make this fairly trivial but are only a thing in unstable as far as I could see and im not sure how theyre implemented/perform (optimistically a separate stack and essentially the same performance). Storing the state in an iterator should be doable as well but I would suspect it Could come with some more runtime overhead but would at least keep us in stable rust land.

[datdenkikniet]

I've added the expand-cube-map-as-iterator thing partially in b9be0ec, please take a look and tell me if that's what you had in mind :) The specific change has the same performance as the commit before it, so only a minute degradation (and not due to turning that specific part into iterators).

Edit: the impl CubeMapPos block should be moved to polycube_reps, but I didn't do that for this commit just yet.

It seems that we can do exactly what you proposed. Just need to move some extra functions in pointlist into impl blocks to make the DX not so terrible.

[NailLegProcessorDivide]

Looks good over all, I think maybe do_cube_expansion itself could return an iterator rather than handling the insertions itself.

With respect to moving to polycube_reps I think polycube_reps (+ rotations) itself could use being broken down into a submodule as it already has 2 polycubes in it and im interested in adding a third, and it is kept completely separate from naive_polycube and raw_polycube.

I dont know how much hierarchy is considered normal for rust project structure but moving all the representations of polycubes into a directory like polycubes/{cube_type}/mod.rs might be nicer going forward.

I'm not sure if you'd agree but my ideal for what the traits could allow would be converting enumerate for naive, rotation, reduced and pointlist to essentially become variants of.

fn naive_enumeration<PolyCube, Container>(initial_set Container, initial_n: usize, target_n: usize) -> Container {
  for i in initial_n .. target_n {
    let res_set = Container::new();
    initial_set.par_iter().for_each(|cube| cube.expand().for_each(|new_cube| res_set.insert()));
    initial_set = res_set;
  }
  initial_set
}

for some Polycube and container type

[datdenkikniet]

I think something like that could be good, yes.

I don't think constraining ourselves to a Container makes a lot of sense, we should definitely stick with Iterators of some kind. If a specific container is required, I feel that that should be an implementation detail.

I think we should merge this so we can continue building on it in different places. If you could confirm that you're on board with that, @NailLegProcessorDivide, I would like to ask @bertie2 to merge :)

[NailLegProcessorDivide]

yes, this looks good to me.

I dont see any current trait for what a polycube is unless im missing something. so I'm currently trying to add one that I can refactor rotation_reduced against.

At the moment I'm struggling to figure out the right combination of traits and generics to make some thing like

trait Polycube {
  fn expansions(&self) -> Iterator;
}

to work because or the restrictions around Sized, dynamic return types and my lack of familiarity with rust patterns. Im trying to learn a bit from what you did a bit in iterator.rs but think I need to have both a templated iterator and polycube that know about each other at least the way I'm currently trying to make it work.

[NailLegProcessorDivide]

I think maybe implementing expansions as something against your existing polycube iterator and just having an iterator that knows about the polycube internals maybe easier but im quite unsure.

[datdenkikniet]

I think we'll need some associated types, and we should be good to go (in a different PR, I think doing the restructuring you propose in #34 first/simultaneously makes a lot more sense).

Maybe something like:

// From and Into bounds are optional, but probably useful?
pub trait Polycube: From<RawPCube> + Into<RawPCube> {
    // An associated type describing the iterator used for expansions.
    // Add lifetime so that `expansions` may borrow `self`.
    // Ideally, this would be `-> impl Iterator<Item = Self> + '_` on
    // `fn expansions()`, but we can't do that on stable yet
    // (see: https://github.com/rust-lang/rust/issues/91611)
    type ExpansionIterator<'a>: Iterator<Item = Self> + 'a
    where
        Self: 'a;

    fn expansions<'a>(&'a self) -> Self::ExpansionIterator<'a>;

    fn canonical_form(&self) -> Self;

    // Maybe, for good measure?
    fn size(&self) -> usize;
    fn dims(&self) -> Dims
}

but I'm not sure how to make this more useful. I think the individual representations are quite heavy on implementation details, which is kind of hard to abstract over unless we sacrifice quite a bit of performance.

[datdenkikniet]

@bertie2 this is ready for merging :) Feel free to squash it if you wish to not pollute the history too much.