gap-packages / qpa

GAP package for quivers and path algebras
https://folk.ntnu.no/oyvinso/QPA/
GNU General Public License v2.0
30 stars 13 forks source link

New Operations related to String Algebras #58

Closed BhargavKK closed 2 years ago

BhargavKK commented 3 years ago

Hello, I (BhargavKK) and Amit Kuber (expinfinity) have been working on new operations related to string algebras for past few months. I have added some of its codes in this pull request. I have also added relevant documentation in the 'doc' folder in the 'Path Algebras' chapter, section 4.12. The ideas and concepts used in the codes have been inspired from this paper - https://arxiv.org/pdf/2008.06005.pdf

We are looking forward to comments and suggestions for improvement.

sunnyquiver commented 3 years ago

First of all, thank you very much for the contribution to QPA! I have wanted to get time to understand the code, but I have never gotten to it. So I hope that you have time to answer some (probably stupid) questions? When I read the documentation, it says somewhere that one is limited to 26 arrows. How is this limitation used, and why is it important?

BhargavKK commented 3 years ago

Thank you so much for the reply. For representing the arrows of a quiver, we are using Roman alphabets. The small Roman alphabets are for labeling direct arrows and their corresponding capital alphabets are for labeling their inverse arrows. This puts the restriction of maximum 26 arrows. Moreover, the computation of the operations takes significantly more time, if number of arrows are more than 10. So, this restriction seemed reasonable to us.

I also wanted to draw your attention to one more thing. While passing a quiver as an argument in any of the operations, the arrows of the quiver must be defined in an ascending order starting from 'a' without skipping any alphabet. For e.g. if Q1 := Quiver(3, [ [1,2,"a"], [2,3,"b"],[2,2,"c"] ]); and Q2 := Quiver(3, [ [1,2,"a"], [2,2,"c"],[2,3,"b"] ]); then Q1 would give valid results for the new operations, but Q2 will not.

sunnyquiver commented 3 years ago

More questions:

  1. I see that some functions like Incoming_Arrows are defined in several locations in the code. How do it differ from the attribute "IncomingArrowsOfVertex" defined in QPA?
  2. Some operations are defined like "IsSomething". One would usually require or expect that these would only return "true" or "false". When this is not the case, why is it so?
  3. Related to the above: "IsDomesticStringAlgebra" takes two arguments, while I would expect that this should be a property of an algebra. Why cannot this be a function that takes in a quiver algebra and returns "true" or "false"?
BhargavKK commented 3 years ago

1 Yes, you are absolutely right. I have made the necessary changes in "validity.gi" and "stringalgebra.gi" and replaced "Incoming_Arrows" and "Outgoing_Arrows" with existing attribute "IncomingArrowsOfVertex" and "OutgoingArrowsOfVertex".

  1. For the operations "IsValidString", "IsABand" and "IsDomesticStringAlgebra", we have outputs more than "true" or "false" due to some outliers cases. For e.g. if the presentation (Q, \rho) is not a string algebra or for the operation "IsABand", if the "input_str" is not a Valid String, then we thought, for such cases output should be something more than just "false". Nevertheless, we can always go back to just returning "true" or "false".
  2. Initially we tried using only Quiver Algebra as one argument for "IsDomesticStringAlgebra". Although it was possible to extract the quiver from that quiver algebra using "QuiverOfPathAlgebra", we couldn't separate "\rho" (relations) from it which is needed for other operations.
sunnyquiver commented 3 years ago

More questions and comments:

  1. The functions inverse_right, sigma_eps and direct_left (and maybe others) are defined multiple times in the code (at least 2, 3, 3 times, respectively) and each implementation looks identical to the other. Is this the case? I think that one can avoid this by defining a global function through DeclareGlobalFunction (79.10-4) and InstallGlobalFunction (79.10-4), so that one only need to define it once. Is this a possibility? If so, it would make the code more transparent and clearer and in addition make debugging easier in not having to change the code several places instead of only once. Choose names which are not likely to create conflicts with other packages, like QPAStringInverseRight etc.
  2. If you don't want the computation to continue after you get something which is not desirable, then one can also use Error-function, to tell what went wrong.
  3. Given a quiver algebra A one can get hold of the relations via RelationsOfAlgebra( A ). From them you can find the underlying paths in the quiver, it that is what you want?
  4. I am a little bit concerned about that several functions have very general arguments, IsList, IsString. Maybe this is unavoidable. However it is possible to define your own type of objects which might make this aspect better.

Again, thank you very much for all your efforts.

BhargavKK commented 3 years ago

We will look into it and get back to you as soon as possible. We really appreciate your help and suggestions. I will update here as soon as we make necessary changes in the code!

BhargavKK commented 2 years ago

Changes made in the code

  1. Defined global functions "QPAStringQuiverRho", "QPAStringSigmaEps", "QPAStringDirectLeft", "QPAStringInverseLeft", "QPAStringDirectRight" and "QPAStringInverseRight" in "validity.gi".
  2. Have passed appropriate error messages at appropriate places in "validity.gi" and "stringalgebra.gi", The operations "IsValidString", "IsABand" and "IsDomesticStringAlgebra" returns only "true" or "false". All other previous output conditions have been changed to error messages.
  3. The problem actually arises after using "RelationsOfAlgebra (A)". For e.g. Capture We are unable to convert relations into words (["bc", "cd"] in this case).
  4. Regarding general arguments like "IsList" and "IsString", we have taken care of all the cases which could give erratic outputs. For e.g. If the "input_str" in "IsValidString" contains an unacceptable character, then it returns "false" itself. We have also added a category "IsQuiverSA" (name can be changed). The quivers in this category are in accordance with the conditions mentioned in above comment or in the report. We have defined it in "quiver.gi"
sunnyquiver commented 2 years ago
  1. Here is one way of dealing with this problem:
    gap> rels := RelationsOfAlgebra(A);                    
    [ (1)*c^2, (1)*d*e, (1)*e*a, (-1)*b*d+(1)*a*c*d, (1)*e*b*d ]
    gap> rels[4];
    (-1)*b*d+(1)*a*c*d
    gap> onerel := CoefficientsAndMagmaElements( rels[4] );
    [ b*d, -1, a*c*d, 1 ]
    gap> pathonerel := WalkOfPath( onerel[1] );            
    [ b, d ]
    gap> string := List( pathonerel, a -> String( a ) );
    [ "b", "d" ]
    gap> Concatenation( string );
    "bd"

    Will this help you?

Functions defined multiple times: Having functions that are defined multiple times makes the code harder to maintain and less transparent. So one should avoid lengthy functions defined multiple times. Furthermore, if a function is defined multiple times, it might indicate that the function is of independent interest and it might occur in future developments/extensions. Therefore, as soon as a function is defined multiple times, it should be defined as a global function. For example, I still see that computeLPSarray, SourceOfArrow, TargetOfArrow, CyclicPermutationOfBand, NumberOfJoints, and MaximumLengthOFDirectString are multiply defined. Are there others? Furthermore, what is the relationship between SourceOfArrow and SourceOfPath, and TargetOfArrow and TargetOfPath? The function MaximumLengthOFDirectString breaks the "name convention" by using OF and not Of. Accident or on purpose?

If one defines a function within an operation/function, ask yourself if this function is of interest in its own right and deserve to be define as such. Examples here are WeakBridgeQuiver, BandFreeStrings, IsBandFreeString.

Again, thank you very much for all your efforts.

BhargavKK commented 2 years ago

The problem I was dealing with Relations has been solved with the help of your suggestion. Thanks a lot!

  1. Have made all the functions defined multiple times as global functions.
  2. Changed the name of "MaximumLengthOFDirectString" to "MaximumLengthOfDirectString". I don't know how that happened.
  3. I have also changed the arguments of all the functions. I have replaced "Q" (quiver) and "rho" (set of monomial relations) with "A" (an algebra). The code returns error message if "A" is not a string algbera, else continues as usual.
  4. While defining the method "BridgeQuiver", the function "IsBandFreeString" is used in the function "BandFreeStrings" to find all the band free strings and in "BridgeQuiver1" function. So I think this function has to be defined explicitly. Other 3 functions in this method, which are "BandFreeStrings", "WeakBridgeQuiver" and "BridgeQuiver1" are defined in order to simplify the structure of code while examining it. It will probably be easier to understand the algorithm. I can remove "WeakBridgeQuiver" and "BridgeQuiver1" functions, but "IsBandFreeString" and "BandFreeStrings" are necessary, I think.
sunnyquiver commented 2 years ago

Thank you again for all your efforts!

I hope that you can see the report above under the heading "Some checks were not successful". I think that the tests for your contribution needs to be updated to match the modified code. Otherwise the code looks good to me now. In comparison, the first version was approximately 3200 lines, while the latest version is approximately 2150 lines. So if you can change the tests for the code, so that it will pass the checks by GAP, I will then merge your contribution into the master.

BhargavKK commented 2 years ago

Added the test cases

codecov[bot] commented 2 years ago

Codecov Report

Merging #58 (1287b96) into master (32ec391) will increase coverage by 1.51%. The diff coverage is 74.54%.

:exclamation: Current head 1287b96 differs from pull request most recent head 07f2b59. Consider uploading reports for the commit 07f2b59 to get more accurate results

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   58.43%   59.95%   +1.51%     
==========================================
  Files          70       74       +4     
  Lines       21036    22633    +1597     
==========================================
+ Hits        12293    13570    +1277     
- Misses       8743     9063     +320     
Impacted Files Coverage Δ
lib/pathalgpredef.gd 100.00% <ø> (ø)
lib/pathalgpredef.gi 42.97% <ø> (+3.50%) :arrow_up:
lib/pathalgtensor.gd 100.00% <ø> (ø)
lib/algebra.gi 32.65% <20.00%> (-0.22%) :arrow_down:
lib/modulehomalg.gi 69.58% <35.48%> (+0.52%) :arrow_up:
lib/pathalgtensor.gi 88.52% <40.35%> (-6.63%) :arrow_down:
lib/module.gi 66.27% <66.66%> (+<0.01%) :arrow_up:
lib/functors.gi 73.48% <71.42%> (-0.16%) :arrow_down:
lib/validity.gi 74.28% <74.28%> (ø)
lib/stringalgebra.gi 81.69% <81.69%> (ø)
... and 15 more
sunnyquiver commented 2 years ago

All checks passed. Merge with master done.

Thank you very much for your contribution!