SupposeNot / RAMP

Research Assistant for Maps and Polytopes
4 stars 0 forks source link

Change most methods to accept IsManiplex instead of IsReflexibleManiplex #120

Closed CunningGabe closed 2 years ago

CunningGabe commented 2 years ago

Rationale here, pasted from Discord:

I think the fix we want is going to be a little annoying to do right now, there are a bunch of methods that expect IsReflexibleManiplex or IsReflexibleManiplexAutGpRep as input (The latter means that it was specifically built in terms of a quotient of an Sggi, rather than built in some random way and then later seen to be reflexible) Mathematically, that makes sense - some operations only make sense for Reflexible things But the way that method selection is done, GAP only ever uses "known properties / categories / etc" when picking a method. So if you try to ask for UniversalExtension(M), and GAP doesn't know that M is reflexible - then it will throw an error, even if M really is reflexible @MarkMixer (This is also why ChiralityGroup wasn't working on ToroidalMap44([2,0]) when you tried it last week - that map didn't know it was a rotary maniplex) I don't think we actually want an ImmediateMethod here where GAP checks reflexibility whenever the maniplex is built, because that adds a lot of overhead Rather, I think that we probably want most of our methods to just accept IsManiplex and then do checking within them. So UniversalExtension(M) will take any maniplex, and then check if it's reflexible. If not, then it throws an error. Otherwise it moves on merrily. So, the proposal would be to go through and change all of our methods to accept any maniplex and then do finer type-checking in the body of the method. How does that sounds? (And to be clear: I still want to make IsReflexibleManiplex work the way it obviously should)

SupposeNot commented 2 years ago

"So, the proposal would be to go through and change all of our methods to accept any maniplex and then do finer type-checking in the body of the method. How does that sounds?" That sounds good to me, we may want to see if there are some generic testing functions we can build that handle exits gracefully so we aren't having to write essentially the same code 15 different times.

CunningGabe commented 2 years ago

OK, this is mostly done across c454825ca8659a258e97b0e0622a199e5f9207cc, a21b4807bf4bfbaafeeba3c05068bacc19f8bb60, 535766a302fd5112030f12db4c34ab477fdf643f.

A few notes:

  1. Sometimes we have operations that have one version for arbitrary maniplexes, and one for reflexible maniplexes (e.g. NumberOfIFaces). This is fine and in fact good - if M doesn't "know" that it is reflexible, we default to the general method, which is probably what we want rather than incurring the overhead of testing every maniplex for reflexibility right away.
  2. Some code in the middle of chunks checks for IsReflexibleManiplexAutGpRep. This is pretty similar to (HasIsReflexible(M) and IsReflexible(M)). That is, it is used when we want to do something different only if M already knows it is reflexible and knows its automorphism group.
  3. Operations that are only defined on reflexible maniplexes should nevertheless be declared as operating on IsManiplex. Then there can be a quick check for reflexibility at the beginning of the method; see e.g. UniversalExtension in constructions.gi.

I also fixed IsReflexibleManiplex so that, e.g. IsReflexibleManiplex(Maniplex(ConnectionGroup(Cube(3)))) = true. But note that Maniplex(ConnectionGroup(Cube(3))) does not know right away that it is reflexible, which is why we still needed all of the changes above.

I think this is done for now. Let me know if any operation doesn't work on a maniplex that is reflexible but doesn't know it.