Closed eliotwrobson closed 4 months ago
Hi @eliotwrobson,
Thank you once again. Regarding the other issue, I've made significant updates to the count diagonal and Frobenius coordinates methods. Now, I've completely revamped the approach. Initially, I compute the Frobenius coordinates through a loop, utilizing both physicist and mathematician definitions. Once these expressions are derived, I proceed to calculate the diagonal terms based on the length of these coordinates. Additionally, I've implemented some tests to ensure accuracy, and I verify the number of boxes in the diagonal using hook lengths to validate the results. -- Check the dev branch.
I'll revisit the remaining points on Monday. Thanks again
Awesome! I think the new code computing this recomputes the transpose repeatedly in the inner loop, so you should store this in a variable if you can, but otherwise looks much cleaner that before 👍👌
well noticed! fixed.
Hi @eliotwrobson,
Some (in my opinion) positive updates:
Upon reviewing volume 4 of TAOCP, I came across an interesting relationship between the conjugate of a Young diagram and its conjugacy representation. Leveraging this connection, I've revamped the method for calculating the transpose, resulting in a significant improvement.
Additionally, I've addressed the other points you raised. Thank you for those suggestions; I found them quite beneficial, especially the 'pairwise' tip.
I might have overlooked something, but I do agree with the necessity of validating the type. I've implemented it differently, though.
The review has been included in the readme. However, I haven't yet added it to the documentation. I plan to do so in the future; I just need to figure out how to incorporate LaTeX support into Sphinx. It's likely straightforward, but I prefer not to delve into that at the moment.
@thraraujo that all sounds excellent! None of these items were hard review blockers (besides the statement of need), just things I noticed that could possibly clean up parts of the package. If you're satisfied with changes made based on my suggestions, feel free to close this issue 😃
@eliotwrobson, thanks... In fact, I was just checking the statement of need. I didn't like the way sphinx adds math support, so I created a link to the repo. Case closed!
Just some small miscellaneous things I noticed:
isinstance(self.partition, (tuple, np.ndarray))
: https://github.com/thraraujo/pysymmpol/blob/main/pysymmpol/partitions/young.py#L33See https://stackoverflow.com/a/33311330
all
function can be called on a generator expression instead of a list, and using the slice here makes a copy: https://github.com/thraraujo/pysymmpol/blob/main/pysymmpol/partitions/young.py#L39It would be better to have
A = all(x >= y for x,y in pairwise(par))
, avoiding the issues mentioned above.tuple
type annotation, when you really only want tuples that contain numbers. This can be done with the annotationtuple[numbers.Number, ...]
, indicating an indeterminate length tuple containing only numbers: https://stackoverflow.com/a/4187266Similarly specific annotations can be done with a lot of the
dict
annotations in the package as well.count_diagonal
function, you create a whole matrix and then compute the trace: https://github.com/thraraujo/pysymmpol/blob/main/pysymmpol/partitions/young.py#L88However, all you are doing in the inner loop is constructing a row of
partition[n]
ones and padding with zeros, which is wasteful for large diagrams. Instead, you can just iterate through each row of the diagram and add 1 to your final result ifpartition[n] >= n
, indicating that there would be a 1 on the diagonal. In fact, I think this is done by the one-linersum(1 if part >= n else 0 for n, part in enumerate(partition))
, as this adds 1 for each row in the diagram that crosses the diagonal.Part of the ongoing JOSS review: https://github.com/openjournals/joss-reviews/issues/6724