-
-
Notifications
You must be signed in to change notification settings - Fork 96
Fix #682 #684
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
Fix #682 #684
Conversation
Summary of ChangesHello @lilkeet, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a subtle dependency bug where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly fixes a bug where std/strformat was used without being imported. The changes to the test files to remove the now-unnecessary imports are also appropriate. My review includes suggestions to improve the maintainability of the fix by centralizing the bind &`` statement to avoid code duplication across multiple procedures.
| import ../tensor | ||
| import ./helpers/triangular | ||
| import std / [sequtils, bitops] | ||
| import std / [sequtils, bitops, strformat] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid repeating bind &`` in the diagonal, `set_diagonal`, and `tri` procedures, you can declare it once at the module level here. This improves maintainability by removing code duplication. The `bind` statement at the module level will ensure the `&` operator is available for all procedures in this file, including within generics.
import std / [sequtils, bitops, strformat]
bind `&`
| ## - anti: If true, get the k-th "anti-diagonal" instead of the k-th regular diagonal. | ||
| ## Result: | ||
| ## - A copy of the diagonal elements as a rank-1 tensor | ||
| bind `&` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ## - k: The index k of the diagonal that will be changed. The default is 0 (i.e. the main diagonal). | ||
| ## Use k>0 for diagonals above the main diagonal, and k<0 for diagonals below the main diagonal. | ||
| ## - anti: If true, set the k-th "anti-diagonal" instead of the k-th regular diagonal. | ||
| bind `&` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ## diagonal. The default is false. | ||
| ## Result: | ||
| ## - The constructed, rank-2 triangular tensor. | ||
| bind `&` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
The issue is that this will happen for every single generic proc, and all procs are genetic on |
|
Ah but here it's internally that I use strformat. |
Solves issue #682.
The
&proc was used before thestd/strformatmodule was imported.The test suite missed this because it imported the proc itself.