Skip to content

fix default elements for ccbar_asymm basis#1649

Merged
RoyStegeman merged 2 commits into
masterfrom
fix_fitbasis
Dec 21, 2022
Merged

fix default elements for ccbar_asymm basis#1649
RoyStegeman merged 2 commits into
masterfrom
fix_fitbasis

Conversation

@tgiani
Copy link
Copy Markdown
Contributor

@tgiani tgiani commented Dec 13, 2022

V15 was missing from the defualts_elemets of the CCBAR_ASYMM and this was breaking vp-nextfitruncard

@tgiani tgiani requested a review from scarlehoff December 13, 2022 10:16
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Dec 13, 2022

Please add a test somewhere. The way this is done at the moment is changing evolution as well. Not sure that is intended.

@tgiani
Copy link
Copy Markdown
Contributor Author

tgiani commented Dec 13, 2022

ah right

@tgiani
Copy link
Copy Markdown
Contributor Author

tgiani commented Dec 13, 2022

I think it s probably better to redefine the basis altogether? I mean without doing CCBAR_ASYMM = evolution ?

@RoyStegeman
Copy link
Copy Markdown
Member

RoyStegeman commented Dec 13, 2022

You could also just use copy.deepcopy, that way you don't have to define the basis transformation twice.

Specifically `CCBAR_ASYMM = copy.deepcopy(evolution)`. Without deepcopy edits to CCBAR_ASYMM attributes overwrite attributes in evolution basis.
@scarlehoff
Copy link
Copy Markdown
Member

I think this would be good to be merged (and since we are discussing ccbar I'd feel better if we are all in the same page -or branch-)

@Zaharid what kind of tests would you add?

@RoyStegeman
Copy link
Copy Markdown
Member

Yes this should be merged, don't want to keep the current bugged (buggish?) implementation in main.

Also wondering about what tests you had in mind. There's not really a clean way to test it, unless we want to do so for all bases explicitly just to prevent us from doing something like this again. Perhaps instead we should just remove EVOLUTION = evolution (and instead do EVOL = LinearBasis.... ) to make it less likely to accidentaly intruduce bugs like this, or we can also add EVOL = copy.deepcopy(evolution), mostly code is just copy-paste with a minor edit anyway.

@RoyStegeman RoyStegeman added the bug Something isn't working label Dec 19, 2022
@RoyStegeman RoyStegeman merged commit aaf41db into master Dec 21, 2022
@RoyStegeman RoyStegeman deleted the fix_fitbasis branch December 21, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants