refactor polycube_reps and rotations

[NailLegProcessorDivide]

based on #31 moves polycube representations into own submodules

[NailLegProcessorDivide]

Im not sure whats normal within rust but things like opencubes::polycubes::pcube::Compression::None feels like its approaching Java territory

[datdenkikniet]

I agree :P

Depending on the goal of the polycubes module, you could do something like

mod pcube;
pub use pcube::{Compression, PCubeFile, ...}

mod point_list;
pub use point_list::Dim;

instead to encapsulate it a bit.

[datdenkikniet]

Copying from #31 for reference:

// From and Into bounds are optional, but probably useful but not necessary
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>;

    // Handy to have
    fn canonical_form(&self) -> Self;

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

[NailLegProcessorDivide]

I have (now updated based on yours)

pub trait PolyCube: PartialEq + PartialOrd + Hash + Clone + Copy + Debug + Sized + From<RawPCube> + Into<RawPCube> {
    type ExpansionIterator<'a>: Iterator<Item = Self> + 'a
where
    Self: 'a;

    fn new() -> Self;
    fn expansions<'a>(&'a self) -> Self::ExpansionIterator<'a>;
    fn canonicalise(&self) -> Self;
    fn get_cube(&self, x: u8, y: u8, z: u8) -> bool;
    fn set_cube(&mut self, x: u8, y: u8, z: u8);
    fn set_cube_to(&mut self, x: u8, y: u8, z: u8, val: bool);
    fn clear(&mut self);
    fn get_count(&self) -> u8;
    fn get_shape(&self) -> Dim;
    fn is_continuous(&self) -> bool;
}

in my local branch at the moment and would be open to any feed back. I changed a lot of things like dimensions to u8 because its basically impossible to go past 255, i8 may be better for allowing reps that have a concept of a negative coordinate.
Its a bit of a mess from me trying to get things that work so any comments are very welcome. The subtrait (?) spam is pretty much copied from what all my current ones implement

[datdenkikniet]

I think we should carefully consider the final goal of this trait, that's what will tell us if we're over- or underdoing it.

1: In the "brute force" approach (i.e. finding all unique expansions, by brute-forcing solutions and disregarding the fact that we can do things in a specific order), the goal is "we want to take an iterator over input cubes of size n and produce an iterator over all cubes of size n + 1 that we can make from the input".

Ideally, all you'd need is something simple like:

pub trait Polycube {
    type ExpansionIterator<'a>: Iterator<Item = Self> + 'a
    where
        Self: 'a;

    /// Produce an iterator that yields all unique n + 1 expansions of
    /// `input`.
    fn unique_expansions<'a>(input: impl Iterator<Item = Self>) -> Self::ExpansionIterator<'a>;
}

2: For the "smart" approach (i.e. hashless), I see that we more want something like "we want to take an iterator over input cubes of size n and return the amount of children of size m that those cubes have", ignoring the fact that we could store the intermediaries somewhere.

Something like:

pub trait Polycube {
    /// Count all unique expansions of size `target` for all polycubes in `input`
    fn unique_expansions<'a>(input: impl Iterator<Item = Self>, target: usize) -> usize;
}

It feels like all of the other fns are implementation details, in the sense that they could be shared between different impl Polycubes, but they aren't necessary for the end goal.

Additionally, all of the other trait bounds (PartialEq, PartialOrd, etc. etc.) can easily be deferred to some other function that actually has these requirements, i.e.

pub struct PolycubeEnumerator<T> 
    where T: Polycube + Eq + Ord + Hash
{}

// Do stuff that requires Eq, Ord, Hash in an impl block

What is, to you, the goal with the trait you're writing? IMO the main goal is "just" to do enumerations, and the other things can either be implementation details or put into sub-traits that are trait CanonicalizablePolycube: Polycube or something.

Edit: also, #31 was squashed (on my request), so you'll have to drop all of my commits that are on your branch and rebase. (I think resetting to main and cherry-picking your commit is the easiest)

Edit2: ah, and some sort of fn size(&self) -> {u8 or usize} is definitely also handy, so that you can actually reason about it outside of the polycube's implementation.

[NailLegProcessorDivide]

Thats a good point.
For the bruteforce enumeration all we need is the way to get expansions and then some contatiner type that knows how to deduplicate the polycubes and returning them in a serial order, preferably in parallel (for this case could be a property of either the structure or the algorithm?).

for the "smart" DFS approach it then also needs some sort of is_canonical_root which I Agree now could definitely be moved to a different subtrait

I'll fix the history in the branch in a bit my local checkout is a bit of a mess rn

[datdenkikniet]

Yeah, absolutely! I think we'll want to land somewhere in between what I wrote (which, on further consideration, is definitly too sparse to be useful) and your idea a few comments up.

In the end I think something that we can get all expansions (perhaps with a constraint on the ordering to facilitate the DFS search?), the size, and a method of transforming into the canonical representation should allow us to semi-easily write implementations of the brute-force and smart (= DFS) algorithms that is generic over the type of polycube provided.

While writing this I think that sounds like a decent goal. Any opinions?

[NailLegProcessorDivide]

Smart is a bit more involved than just what you put above and I think would be justified needing its own traits.
It needs a way of telling ig the polycube it was just expanded from is its "true" parent or if it is just an alternative derivation.
The way I implemented this was:
taking a copies of the polycube with each subcube removed
seeing if its still a continuous polycube
test if the "minimum" ordered valid parent is the one we just grew it from

In my opinion this means, either we want that algorithm to be part of the cube as I cant tell if its a detail of the algorithm, it could be derived maybe from the components or optionally given a seperate implementation for a polycube,
we could have a trait to validate continuity and cube removal but these could still be annoying and is still quite specific to smart for now.
either way I dont think a minimal Polycube should support smart implementation

[datdenkikniet]

I see! Yes, that does sound much more difficult to make generic in an easy way.

A trait that can do the brute-force, is probably enough for a start, then :)

[datdenkikniet]

I have made a PR NailLegProcessorDivide#1 for your branch, to add some extra improvements to this branch (that hopefully make our lives a bit easier)