Skip to content
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

updated LinearAlgebraForCAP/gap/CompilerLogic.gi #1424

Closed
wants to merge 11 commits into from

Conversation

mohamed-barakat
Copy link
Member

Preparing for the upcoming PreComposeList-PR.

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
LinearAlgebraForCAP/PackageInfo.g 100.00% <100.00%> (ø)
LinearAlgebraForCAP/gap/CompilerLogic.gi 100.00% <100.00%> (ø)
...recompiled_categories/MatrixCategoryPrecompiled.gi 66.52% <ø> (+0.47%) ⬆️

📢 Thoughts on this report? Let us know!.

@mohamed-barakat mohamed-barakat force-pushed the prepare8 branch 3 times, most recently from 5f7358b to 42f43c1 Compare August 16, 2023 12:38
@zickgraf
Copy link
Member

This PR is difficult to review because it introduces so many rules at once, so it's difficult to see how the new compiled code exactly comes from the old code. Some rules might be suboptimal (e.g. KroneckerMat(id, id) -> id is potentially a loss of information, although I guess here it does not cause problems) and others need filters (e.g. 0 * something = 0 does not hold if something is a homalg matrix).

Could you split the PR into smaller parts where the diff is more readable?

@mohamed-barakat
Copy link
Member Author

You mean more several commits or several PRs?

@zickgraf
Copy link
Member

Depends :D If only view commits need further discussions, one PR is fine. If we would create too many threads, I would suggest splitting the PR. We can try with one PR.

@mohamed-barakat
Copy link
Member Author

Depends :D If only view commits need further discussions, one PR is fine. If we would create too many threads, I would suggest splitting the PR. We can try with one PR.

Done. Will update after #1432 is merged since I need the latter more urgently.

@mohamed-barakat
Copy link
Member Author

Done. Will update after #1432 is merged since I need the latter more urgently.

#1433 is hopefully the last one I need before rebasing this PR.

@mohamed-barakat
Copy link
Member Author

Two rules turned out to be obsolete :)

@zickgraf
Copy link
Member

The first few commits only affect MorphismFromFiberProductToSink etc., which I consider niche applications. I'm a bit reluctant specializing too much for such special applications and thus slow down the whole compilation process, especially if the optimizations could/should be implemented much more generically. For example, the first two rules could be handled generically by "If an expression does not depend on variables and is a nested list of literals (e.g. integers), evaluate it and use the result." Here, "using the result" means encoding the result as a syntax tree again, which is something you already do in the context of the ur-algorithms, I think.

Maybe we can defer those optimizations only affecting niche applications for now?

@mohamed-barakat
Copy link
Member Author

Maybe we can defer those optimizations only affecting niche applications for now?

I am not sure if the first few logic templates only affect the operations you mentioned. It might be an artefact of the order of the commits.

@mohamed-barakat
Copy link
Member Author

The first few commits only affect MorphismFromFiberProductToSink etc., which I consider niche applications. I'm a bit reluctant specializing too much for such special applications and thus slow down the whole compilation process, especially if the optimizations could/should be implemented much more generically. For example, the first two rules could be handled generically by "If an expression does not depend on variables and is a nested list of literals (e.g. integers), evaluate it and use the result."

You were right. The logic templates that did not depend on variables were only necessary to simplify MorphismFromFiberProductToSink etc.

Furthermore, I had to defer one rule, until its effect on the rest of the code becomes applicable.

@mohamed-barakat mohamed-barakat force-pushed the prepare8 branch 2 times, most recently from 35f3348 to 2ec5219 Compare August 20, 2023 12:48
@zickgraf
Copy link
Member

Thinking more about this, I do not understand why the PreComposeList-PR should change any compiled code (except the one of PreComposeList of course). I don't think the additional source and range are used except for the empty case, but this case does not occur in the existing code. So maybe the changes were an artifact of the implementation of PreComposeList via if-else instead of using Iterated with three arguments?

@mohamed-barakat
Copy link
Member Author

So maybe the changes were an artifact of the implementation of PreComposeList via if-else instead of using Iterated with three arguments?

You are right :)

@mohamed-barakat
Copy link
Member Author

No need to merge this branch now. I just rebased to check it is still passing the CI.

@mohamed-barakat
Copy link
Member Author

After merging #1492 I had to update the PR. Commit c8f27fb made one commit in this PR obsolete.

@mohamed-barakat
Copy link
Member Author

Superseded by #1615.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants