ulthiel / JuLie.jl

Mathematically sound structures and fast algorithms for things around representation theory, especially algebraic Lie theory and accompanying combinatorics.
GNU General Public License v3.0
6 stars 7 forks source link

Added num_conjugacy_classes and some tests #35

Open dmathiae opened 3 years ago

ulthiel commented 3 years ago

Thanks. This needs some modifications/checking:

Further, does it make sense to have an own function num_stuttering_multipartitions in multipartitions.jl for later use?

fingolfin commented 3 years ago

Also, perhaps drop all those test commits?

ulthiel commented 3 years ago

Yes. Can I do this or does Dario have to do this?

dmathiae commented 3 years ago

On 11. Feb 2021, at 10:11, Ulrich Thiel notifications@github.com wrote:

Thanks. This needs some modifications/checking:

I am not sure about your floor(t/k)!=t/k because this uses float which for us may potentially break things. Yes, I looked around but haven't found anything that checks divisibility... Don't convert back to Int64 when counting stuff. The m,p,n are just Int64 because I don't think you'd ever go bigger. But counting stuff may overflow. num_multipartitions uses fmpz for the binomial function and appending throws an error when trying to combine integers and fmpz into an array. The function should be called num_classes. That I can fix. I don't understand the comment on "removing duplicates". Well, a d-stuttering partition is e-stuttering for any e that divides d, so you have to subtract the entries in the array whenever one index divides the other. Further, does it make sense to have an own function num_stuttering_multipartitions in multipartitions.jl for later use?

I don't think so, since that part is easy to reproduce (just num_multipartitions(n/d,m/d), and I've only encountered d-stuttering with respect to CM-parameters, for which you couldn't use parititons.

About the nonsense commit messages, I could say that I don't believe in erasing the mistakes of the past, but I just couldn't figure out how to do it without nuking everything.

—Dario.

ulthiel commented 3 years ago

Yes, I looked around but haven't found anything that checks divisibility...

What about t % k != 0 ?

num_multipartitions uses fmpz for the binomial function and appending throws an error when trying to combine integers and fmpz into an array.

Not sure, maybe you should initialize a = fmpz[] ?

Well, a d-stuttering partition is e-stuttering for any e that divides d, so you have to subtract the entries in the array whenever one index divides the other.

Add this comment to the comment.

ulthiel commented 3 years ago

I think instead of append! you also want to use push!

fingolfin commented 3 years ago

Mistakes of the past should not (must not!) be erased once they are on master.

but in a PR? Absolutely!

i suggest to learn about "interactive rebase" it makes it very easy, especially in the current case. Google a bit? I am happy to also help or explain ina video meeting

ulthiel commented 3 years ago

Yes, Max knows how these things are supposed to look like, and I agree! Sorry, Dario.

ulthiel commented 3 years ago

I'm currently implementing a new type hierarchy around complex reflection groups. As you just have a single function, it won't be a problem to integrate that but the merge may not work when I'm ready. We'll see.