-
Notifications
You must be signed in to change notification settings - Fork 18
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
Verify that all derivations have correct source and range #1239
Comments
I'm currently working through the list your code generates and several derivations are from |
Nice :-)
In theory, yes. In practice I'm not working on this right now and there are still some engineering problems to be solved, so there is no timeline for this from my side.
"Skip" in the sense of "not look at" or in the sense of "not fix"? If you find an error in the closed case, it would be very nice if you could also fix the coclosed case. When applying #1468 for the first time, one should compare the autogenerated derivations to the existing ones to catch errors like #1465. So any error/inconsistency fixed beforehand makes this comparison easier. |
I'll give you an example of what I mean: the following derivation is missing a lot of CAP_project/MonoidalCategories/gap/SymmetricClosedMonoidalCategoriesDerivedMethods.gi Lines 728 to 757 in d726b24
It's dual derivation has the same issue: CAP_project/MonoidalCategories/gap/SymmetricCoclosedMonoidalCategoriesDerivedMethods.gi Lines 740 to 771 in d726b24
This dual derivation works fine and has no bug (I think). So I hoped, if I add these All of what I encountered until now in
I'm fine with doing both cases by hand if you advise this; it's just quite time consuming and error prone, so I hoped to be more efficient. :) |
Ah, now I understand. I think modifying only the closed case but not the coclosed case is not a good idea: This introduces unexpected asymmetries which in turn can lead to weird issues like #917.
I see the following options:
Does this derivation show up as having the wrong source or range? Since the last function call is a WithGiven operation, I expect no problem here. |
Then I'll also make the corresponding coclosed changes (for everything I changed until now) to avoid asymmetries and then give the automatic dualization a try. If I can't get it to work, I'll continue doing it manually.
At least its range is fine, I haven't checked sources, yet (but I guess here the source should be fine, too). What triggered an error is the comparison of compiled code at the end of your code (in the range case). I think because not all operations could be resolved, because of missing |
Something else I just noticed: many (all?) operations which return an isomorphism (e.g. |
I think I have found the culprit: You are probably using zickgraf@e0b9edf as-is, right? That's a very old commit. @mohamed-barakat has recently added many WithGivens in b6e2ea5. You can find a rebased version of my commit by looking for
Yes, those isomorphisms are mainly used for deriving one concept from another, including objects, e.g. DirectProduct from DirectSum. In this context, WithGiven versions do not make sense, because you obviously cannot derive DirectProduct from ...WithGivenDirectProduct. |
Thanks! |
@TKuh FYI: I have removed the commit containing |
using the code in zickgraf@e0b9edf.
Currently, there are many false positives because of missing
output_source/range_getter_strings
(which means CompilerForCAP cannot determine the source/range of an operation) and because of missing WithGiven versions of operations (which means the given source or range cannot be passed through to the result).The text was updated successfully, but these errors were encountered: