search

ValeevGroup / SeQuant

SeQuant: Symbolic Algebra of Tensors over Operators and Scalars
GNU General Public License v3.0
17 stars 6 forks source link

subspaces constructed with ignore spin on lead to faulty index compar… #224

Closed cmasteran1 closed 3 months ago

cmasteran1 commented 4 months ago

Whenever possible please do not ignore spin when constructing subspaces. some functions (like spintrace) construct spinfree indices with Spin::Any QNS. This lead to a case where two identical indices were not identified as such because they had different QNS. fixes #212

Krzmbrzl commented 4 months ago

Thanks for looking into this! :+1:

What is the intended use case for the ignore_spin option?

I'm wondering whether this fixes the underlying issue or only the symptom 🤔

cmasteran1 commented 4 months ago

As I understand it, the option is only needed to ensure certain unit test canonicalization stays consistent. Considering the option is only present for generating legacy subspaces, I don't think it will be an issue going forward.

Krzmbrzl commented 4 months ago

Okay, I see. I did search the code base for where this is currently used and only found https://github.com/ValeevGroup/SeQuant/blob/1095dda5c59cbb4aa7793435f02427cc3696622a/tests/unit/test_spin.cpp#L731-L734 which is needed in order for https://github.com/ValeevGroup/SeQuant/blob/1095dda5c59cbb4aa7793435f02427cc3696622a/tests/unit/test_spin.cpp#L749-L774 to pass

and https://github.com/ValeevGroup/SeQuant/blob/1095dda5c59cbb4aa7793435f02427cc3696622a/tests/unit/test_spin.cpp#L566-L569 which is needed to make https://github.com/ValeevGroup/SeQuant/blob/1095dda5c59cbb4aa7793435f02427cc3696622a/tests/unit/test_spin.cpp#L585-L601 pass.

To me it is somewhat worrysome that some of the test cases for seemingly foundational functionality only pass with this particular hack :eyes:

@evaleev do you know why this hack is needed?

evaleev commented 3 months ago

@evaleev do you know why this hack is needed?

I added a note (WARNING) explaining the need for this ... potentially easier solution is to change the reference results but it feels good to have a test that uses different spin attribute representation ...