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

Enforce default for matching order at post-init time #326

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

alecandido
Copy link
Member

Hot fix for matching order setting.

It should be properly tested, but if merging this is urgent we can postpone it.

src/eko/io/runcards.py Outdated Show resolved Hide resolved
@alecandido alecandido force-pushed the matching-order-default branch from ae952e5 to 0c4bf11 Compare November 21, 2023 14:28
@alecandido
Copy link
Member Author

alecandido commented Nov 21, 2023

It should be properly tested, but if merging this is urgent we can postpone it.

I take it back: thanks to @felixhekhorn is already tested. And the behavior was different from the one I imagined.

Actually, this would not work with the polarized settings @RoyStegeman forwarded (the default would not reproduce those settings, but you should keep manually specifying it).

theorycard = TheoryCard.from_dict(
    {
        "order": [2, 0],
        ...
        "matching_order": [2, 0],
    }
)

(and instead it will be "matching_order": [1, 0], by default)
However, before fine-tuning for polarized, I'd like to understand why polarized should be different from unpolarized. @Radonirinaunimi

@RoyStegeman
Copy link
Member

if merging this is urgent we can postpone it

It's not urgent since I can just set the matching orders manually. Nevertheless, I think this can be merged once @Radonirinaunimi is able to confirm if this is also correct for polarized.

@felixhekhorn
Copy link
Contributor

  • I'm slightly worried about unintended side effects (i.e. please wait and check the benchmarks)
  • we should drop these lines
    if tcard.matching_order is not None
    else (tcard.order[0] - 1, 0),
    to avoid double acounting
  • it is fine to keep this one
    new["matching_order"] = old.get("PTO_matching", [old["PTO"], 0])
    since we may have PTO_matching (although in real life, I think, this never happens ...)

@giacomomagni
Copy link
Collaborator

  • we should drop these lines
    if tcard.matching_order is not None
    else (tcard.order[0] - 1, 0),

    to avoid double acounting

I thought the same, but one should be to evolve ekos without that key and the other one for reading ekos without that key no?

@felixhekhorn
Copy link
Contributor

I thought the same, but one should be to evolve ekos without that key and the other one for reading ekos without that key no?

Mmm not really, I think - either case would create a TheoryCard object and after calling the constructor you're done

@alecandido
Copy link
Member Author

Indeed, @felixhekhorn is right: they are acting both at the level of the TheoryCard, so they are doubled.
The new one is for reading and writing, since acting directly on TheoryCard, the old one only for writing, since enforced during computation.

@alecandido alecandido force-pushed the matching-order-default branch from 0c4bf11 to 95299fd Compare November 22, 2023 09:09
Co-authored-by: Felix Hekhorn <[email protected]>
@felixhekhorn
Copy link
Contributor

Can this be merged?

@alecandido
Copy link
Member Author

For me, it was ready. If you and @giacomomagni agree, then I'll merge.

@alecandido alecandido merged commit 430f9e1 into master Dec 6, 2023
6 checks passed
@alecandido alecandido deleted the matching-order-default branch December 6, 2023 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants