Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
DrPaulSharp
force-pushed
the
refactor_24_linalg
branch
from
84bbb4b to
51bffaf
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
I'm conflicted on this one and will need to think more on it. Overall, it seems fine, but I think I need a real use case for having these operations with the data object. Is the plan to apply them to SAS data?
requirements.txt
Outdated
| # Calculation | ||
| numpy | ||
| scipy | ||
| sympy |
There was a problem hiding this comment.
Adding another requirement is never ideal, especially for something we've never had to do before. If we haven't had to do this operation previously, is there a good justification for adding it now?
What are the real world use-cases for adding these operations?
sasdata/quantities/quantity.py
Outdated
| return np.linalg.det(self.a.evaluate(variables)) | ||
|
|
||
| def _derivative(self, hash_value: int) -> Operation: | ||
| return Trace(sp.adjugate(self.a) * self.a._derivative(hash_value)) |
There was a problem hiding this comment.
Adding an entire dependency for a single use seems excessive. Are there other operation we might use the package for in the future?
There was a problem hiding this comment.
Hmm, I'm inclined to agree overall. It's better to use this dependency than write code for the adjugate operation but we only need it here and I've added the determinant as something that might be useful rather than having a definite use case. I can take the determinant operation out and put it on ice for now and we can put the rest in if that sounds like a good plan?
There was a problem hiding this comment.
No quality gates enabled for this code.
See analysis details in CodeScene
Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
2 participants
This PR adds linear algebra operations for quantities. Note that it introduces a new dependency,
sympy, which has the BSD licence. https://www.sympy.org/en/index.html