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

Fix showing conjugate variables #288

Merged
merged 2 commits into from
Dec 16, 2023
Merged

Conversation

projekter
Copy link
Contributor

name_base_indices might return a Symbol instead of a String, in which case we cannot iterate over it; so make sure that we always have a String.

@blegat
Copy link
Member

blegat commented Dec 16, 2023

Can you add a test ?

@projekter
Copy link
Contributor Author

Maybe? I found this issue when implementing my own primitive realization of MP, which is supposed to do an extremely limited set of arithmetic operations in a very fast and non-allocating way, so I ended up using a Symbol for name_base_indices. However, I did a quick scan of GitHub and it seems that the only package (not on Yggdrasil) that uses this particular return type is YAPP, which is version-bound to MP 0.4 (and I have no idea what its purpose is). So there's not really a way to test it without an implementation using it...
Of course, this is kind of a problem of MP anyway, as it can't do much without using a implementation, so I could add a test that just checks the output of name_base_indices, but it wouldn't have caught this issue had it been present before.

@blegat
Copy link
Member

blegat commented Dec 16, 2023

You can just do

struct SymbolVar <: MP.AbstractVariable end
MP.name_base_indices(::SymbolVar) = (:x, (1,2))

in the tests

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@blegat blegat merged commit 3c5b774 into JuliaAlgebra:master Dec 16, 2023
6 checks passed
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