New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wrong contains in PartitionDiagrams #25662
Comments
comment:1
I do not understand these doctests:
Why is Put differently, the |
Commit: |
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
|
comment:5
ping? |
comment:6
ping ping? |
comment:7
Started reviewing:
I think here
I have no glue when the order is not in ZZ (apperently it can have half values, but is this always the case? (I am a bit scared about this code...) So, either someone tells me, where this is explained or someone else takes a look (I'd prefer the latter). Rest looks good AFAICS. |
Reviewer: Daniel Krenn |
comment:9
Wow, that was quick - thank you! OK, I admit I don't know about the first one, so I simply tried and it looks OK. If I understand correctly, The second one is by design of this class (which is not myself). For the partition algebra, it makes algebraically a lot of sense to have half-integer order. Combinatorially, this simply implies that the final elements of the top and the bottom row, by convention k and -k, are in the same block. By design, all other diagram algebras are currently regarded as sub-algebras of the partition algebra. This may be up to debate, but is certainly outside the scope of this ticket. Martin New commits:
|
comment:10
Replying to @mantepse:
The thing is that
Ok. So then the code is somehow fine for me (because if for whatever reason the order is not half-integer, then there will be an error and not a false result). It might be worth considering restructuring the code so that
Clear, this is not for discussion here. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:13
Done! |
comment:14
Thanks, LGTM. |
comment:15
There are a number of things wrong or misleading. First, I think what you should do is actually properly fix the |
comment:16
As with your comment on the permutation code, I am completely lost. Maybe you could provide a patch yourself, so I can see what you mean? Patching the individual |
comment:17
Replying to @mantepse:
No because there is no patch to provide. What I am saying is that I do not think your proposed change is the way forward with reasoning why. Although I did see one typo above that I have fixed.
It is definitely the way to go because it has better separation-of-concerns (checks are localized to each part), it is faster, and it should not be hard to do. If you want to make it work, then fix it properly. |
comment:18
OK, I'll abandon this for the moment, I don't understand what you want me to do (except for implementing a method for each diagram algebra separately, which I believe is wrong). I'll do the quick fix in the permutations file, but this one is too much work right now. Exams coming up. |
Changed author from Martin Rubey to none |
comment:19
Replying to @tscrim:
I am not sure either, why you think the proposed code is so bad. Isn't having a quite general implementation favored over individual implementations for each class. (But it might also be that I do not really understand your concerns; so, could we elaborate on this...?) |
comment:20
Replying to @dkrenn:
The |
comment:21
A few questions, comments and a clarification: 1.) I actually did propose to implement efficient methods once the generic check works, and as need arises. As you know I spent quite a bit of time speeding up the code for diagram algebras, because it was too slow for my computations. Thus, I think you could trust me that I care about speed and thought about how to achieve it. Fun fact: removing the "specialised"
I am guessing that this is because it first checks whether the candidate is in 2.) I do think the general rule is to go for correctness first, and then optimize. (This is not saying that my patch is perfect, of course. In particular, I think I should have removed the In the case at hand I think it is particularly important to have a well working generic check, to make it less work to implement new diagram algebras. In particular, #25637 moves three from I do not see what "separation-of-concerns" has to do with my approach: I am simply determining containment by trying to construct the diagram. 3.) I understand that you think "it is definitively the way to go", but I do not see why. I think your argument about speed is invalid, because the current implementation is buggy, and duplication of code is often a source of bugs. |
comment:22
Replying to @mantepse:
Martin, I know you care about speed. Patronizing me is not useful to the discussion.
Probably. That is an interesting data point.
IMO that is even more a step in the wrong direction.
I feel that is a bit of a misrepresentation of those
You are taking things that should be in separate classes and pulling them to one class. The containment check for being a Brauer diagram should be done by the Brauer diagram parent class after it has been determined that it is a partition diagram by the partition diagram class. Each class tests what it is suppose to be.
That does not make it invalid. It just means we need to fix a bug. We are only arguing about how to fix the bug. IMO, your fix makes the code less clear, harder to optimize in the future (as any optimization would undo your change), and likely generically makes the code slower (but I have not run timings yet). There is also very little duplication of code. The only part that is duplicated is the part that converts a non (abstract) partition diagram to an object of the parent. If you are worried about that, you can make that into a function. However, the meat of the issue has no duplication. I wonder if the only thing that is going wrong is this part:
|
comment:23
Replying to @tscrim:
reciprocally.
No. Those classes (rooks, planar rooks and permutations) are migrated to the better framework in
No. This is absolutely not what the patch does. I am only removing code. Besides:
No. I agree it is a good idea to check this corner case! Anyway. My offer is as follows:
A final question:
I don't really know. The logic is (for my brain) scattered over too many places.
However, I think it would make sense to swap the The advantage is speed, the cost is that one probably looses some information for the error message. For example, The other way to gain performance and remove duplication of code is probably to remove |
comment:24
Replying to @mantepse:
Yes it is. That is precisely what you are doing. Everything goes into the base class's
This is the sticking point and what I disagree with doing. How about we fix the actual bug?
#25637 is basically independent of this and those old classes are meant to be deprecated. So that is irrelevant to this discussion.
This is not a good strategy. Fix the problem at hand. It sounds like you are suggesting to remove code just to add it back in. If you are not sure and feel it warrants a full design discussion, then let us fix the bug using the current framework, which should be easy, and then have the discussion.
Are you saying you are unable to debug this with the current implementation?
I generally agree with this, but your proposed change actually goes the opposite direction of this.
In properly optimized code, there would be no speed advantage to having a generic method. Generic code needs to work generically, and so it cannot take advantage of special structure.
Removing ABCs is usually independent of performance, and it generally involves more code duplication because there is less commonality with related classes. |
In 8.3 beta 7:
The fix is to remove the individual
__contains__
methods, and correct the generic one, and optimize afterwards. However, to avoid a mess, I'd like to wait for the dependencies.Depends on #25659
Depends on #25462
Depends on #25642
CC: @alauve @tscrim @zabrocki
Component: combinatorics
Branch/Commit: u/mantepse/wrong_contains_in_partitiondiagrams @
ab58ad8
Reviewer: Daniel Krenn
Issue created by migration from https://trac.sagemath.org/ticket/25662
The text was updated successfully, but these errors were encountered: