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

Reorganize runcards and metadata #220

Merged
merged 50 commits into from
Mar 31, 2023
Merged

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Feb 28, 2023

#217 (comment)

Given that I had to rediscover the hot water in #217, I decided that I really needed to solve first #205, i.e. to stabilize the runcards layout.

I knew already from @andreab1997 feedback that there are some suboptimal components (e.g. presence of Rotations in OperatorCard, or the doubling of _mugrid/_mu2grid, none of which should be dumped with a prepended underscore), and I know by myself that some other objects are cumbersome (e.g. the quarks props require a dict with keys "cbt", or the value/scale quantities), or potentially wrongly located (I moved EvolutionMethod to the OperatorCard, along the other properties specifying it, but I never asked everyone else).
Since we have to start soon computing expensive EKOs, it is better to solve this immediately.

So, please, @andreab1997, @giacomomagni, and @felixhekhorn provide feedback on the current status of runcards, such that I can make a proposal for the final layout (of course also @niclaurenti and @t7phy are invited to comment, but I expect the played much less with these runcards).

  • to replace some representations, hiding the underscored objects (potentially assigning default values, and adding some post validations) and
  • to drop the Rotations from the OperatorCard
  • update mu2grid to host tuples with flavor
  • implement flavored_mu2grid()
  • check/fix actual benchmarks
    • check, they are broken
    • but already broken in master, they will be fixed there
  • check Generic based types are working, drop superseded dynamic generation explanation Reorganize runcards and metadata #220 (comment)

@felixhekhorn felixhekhorn added refactor Refactor code output Output format and management labels Mar 8, 2023
@alecandido
Copy link
Member Author

alecandido commented Mar 8, 2023

@felixhekhorn as usual, tests are broken in surprisingly complicate ways, so I'm a bit stuck fixing them...
(in particular, as usual, the most complicated ones are msbar_masses, it is kind of a curse...)

However, the plan, after fixing tests, it is just:

  • to replace some representations, hiding the underscored objects (potentially assigning default values, and adding some post validations) and
  • to drop the Rotations from the OperatorCard

@alecandido
Copy link
Member Author

Now tests are passing thanks @felixhekhorn (after 3 trials of running CI, thanks @codecov...).

@felixhekhorn
Copy link
Contributor

after 3 trials of running CI

If I understood correctly the blog post you linked the other day there is nothing we can do (except move away from CodeCov) and trying to re-run makes the situation only worse ... so if we fail, we can check whether we fail in pytest or on codecov and simply ignore the latter (and hope for the next commit)

@alecandido
Copy link
Member Author

alecandido commented Mar 10, 2023

If I understood correctly the blog post you linked the other day there is nothing we can do (except move away from CodeCov) and trying to re-run makes the situation only worse ... so if we fail, we can check whether we fail in pytest or on codecov and simply ignore the latter (and hope for the next commit)

I tried to update the action version, but as you said in principle nothing should change...
(I had to it anyhow, so let's see)

@alecandido alecandido force-pushed the rearrange-runcards-metadata branch from 662e4c0 to a5ab866 Compare March 10, 2023 11:48
@alecandido alecandido changed the base branch from create-recipes to master March 10, 2023 11:57
src/eko/io/types.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member Author

In c5f0d19 I actually dropped more than I wanted to do in a single commit, but it was worth to fix tests once and for all.
So:

  • pids are now stuck at the flavor basis, since is the only one supported for computation
  • _mugrid and _mu2grid are not any longer present, maintaining them was more expensive than beneficial; from now on, all scales outside EKO should be linear
  • Rotations has been finally removed from the OperatorCard, while still present in the Metadata

There is another breaking change I'd like to operate in this PR (other than the new mugrid with flavor numbers): at the moment, the operator works internally with squared scales.
This is perfectly fine, since we know inside EKO all scales should be squared.
But there is just one weird point: accessors of an EKO object are also using squared scales, so the user should type eko[mu2], when he requested a mugrid.

While this is going in the correct direction, i.e. you decide a scale, and then you square it (that is more predictable than a square root), it's still unpleasant...
Would you mind if I start converting also them for the end user? Internally, it will still use squared scales, and I can add some "squared methods" for the internal usage (but of course there is only one __getitem__, since type info does not survive on values).

@felixhekhorn @giacomomagni @niclaurenti @andreab1997

@alecandido alecandido requested a review from felixhekhorn March 10, 2023 18:46
@alecandido
Copy link
Member Author

alecandido commented Mar 10, 2023

Just to point out once more: our unit tests are not unit at all... and for this reason some specific changes are a real pain...

We definitely need better interfaces and more decoupling between components.

@alecandido alecandido mentioned this pull request Mar 12, 2023
2 tasks
@alecandido
Copy link
Member Author

@felixhekhorn you'll be happy: upgrading mu2grid had an effect (often indirect) in many other places, so a bunch of files had to be changed.

On top of that, since I was already looking at most of the occurrences, I decided to drop Q2grid everywhere (compatibly with legacy mode).
The two things together resulted in a lot of files to be changed. But most of the changes should be trivial, so, despite the number, it should be rather simple to review, even though not the easiest task of course (but we knew from the beginning that this was going to be a breaking PR...).

@alecandido
Copy link
Member Author

There are only two new items that have been generated by 6b830a9:

  • check/fix actual benchmarks: I'd like to use mugrid, consistently with the policy of having linear scales outside EKO, but this might have been done inconsistently, and we do not have benchmarks running in the CI, so the green tick might be misleading
  • implement flavored_mu2grid(): this is almost a requirement for the former one, since in most cases we do not want to specify mu_nf in benchmarks, since there is no choice for LHA, APFEL, PEGASUS, ... but from now on the choice is available for EKO! Thus, we need to split the number of flavors computation outside the runner*, and make it available for others

* actually, the runner will do nothing less because of this, but only "more", since the paths computed by the atlas should take into account the final number of flavors, instead of inferring it

Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

a few comments

src/eko/io/legacy.py Outdated Show resolved Hide resolved
src/eko/io/runcards.py Outdated Show resolved Hide resolved
src/eko/io/runcards.py Show resolved Hide resolved
src/eko/io/runcards.py Show resolved Hide resolved
src/eko/quantities/__init__.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member Author

@felixhekhorn the PR should be ready, but for testing (and possibly fixing) actual benchmarks.

Your review, and potentially also your help (to speed up), would be appreciated 🙇

@alecandido
Copy link
Member Author

Actually, I'm not sure about the representation of the CouplingsRef as a running object:

eko/src/eko/io/types.py

Lines 138 to 146 in 9fc14ba

@dataclasses.dataclass
class CouplingsRef(DictLike):
"""Reference values for coupling constants."""
alphas: ScalarRef
alphaem: ScalarRef
max_num_flavs: FlavorsNumber
num_flavs_ref: Optional[FlavorsNumber]
r"""Number of active flavors at strong coupling reference scale.

There are possibly two running objects, but they have to be coordinated: either the second is not running, or they have to share the reference scale, enforced in __post_init__:

eko/src/eko/io/types.py

Lines 153 to 161 in 9fc14ba

def __post_init__(self):
"""Validate couplings.
If they are both running, they have to be defined at the same scale.
Usually :attr:`alphaem` is not running, thus its scale is set to nan.
"""
assert self.alphas.scale == self.alphaem.scale or isnan(self.alphaem.scale)

But instead for num_flavs_ref we (i.e. I) chose a different strategy: if there is, the number of flavors at reference scale is a unique one.
I wonder if it is better to have separate reference values for alphas/em, with a further single scale (always present since alphas is always running), and then a further field to mark if alphaem is running or not.

This option would denest the structure, save a check (since it will be only possible to represent consistent objects), improve symmetry with num_flavs_ref, and we can still restore the running attributes as constructed properties.

What do you think about? @felixhekhorn @giacomomagni @niclaurenti @andreab1997

@niclaurenti
Copy link
Contributor

niclaurenti commented Mar 19, 2023

I wonder if it is better to have separate reference values for alphas/em, with a further single scale (always present since alphas is always running), and then a further field to mark if alphaem is running or not.

You mean that you would like to remove QrefQED and restore the flag alphaem_running?
For me it's fine, but in this case we should drop the idea of implementing the decoupled running as I proposed few weeks ago.
In ant case, I don't think we really need the decoupled running given that I wanted to implement it only for benchmarking against APFEL.

@alecandido
Copy link
Member Author

alecandido commented Mar 19, 2023

You mean that you would like to remove QrefQED and restore the flag alphaem_running?
For me it's fine, but in this case we should drop the idea of implementing the decoupled running as I proposed few weeks ago.
In ant case, I don't think we really need the decoupled running given that I wanted to implement it only for benchmarking against APFEL.

Ok, good that you mentioned: what is the decoupled running?

Everything that is currently supported will still be supported in the proposed layout. That class is only used to specify the reference value, and we always said that if both are running, the two reference values have to be given at the same scale (since it's the border condition of a vector-valued differential equation).

If you then want to have more of those for some reason, you should be free to instantiate multiple ones per run.
But yes, the proposal is exactly what you said above (i.e. remove QrefQED and restore the flag alphaem_running).

Copy link
Collaborator

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

For my knowledge are these changes compatible with the current Eko on master??
In the sense that ekos can be loaded and used safely, provided pineko will be adjusted...

src/ekomark/data/operators.py Outdated Show resolved Hide resolved
src/eko/runner/compute.py Show resolved Hide resolved
@niclaurenti
Copy link
Contributor

niclaurenti commented Mar 19, 2023

Ok, good that you mentioned: what is the decoupled running?

It's the running without the mixed terms (i.e. the QED corrections to the QCD beta function and the QCD corrections to the QED beta function). So in this case the two Qref can be different.

If you then want to have more of those for some reason, you should be free to instantiate multiple ones per run. But yes, the proposal is exactly what you said above (i.e. remove QrefQED and restore the flag alphaem_running).

In any case, I'm perfectly fine with your proposal.

@alecandido
Copy link
Member Author

alecandido commented Mar 19, 2023

For my knowledge are these changes compatible with the current Eko on master?? In the sense that ekos can be loaded and used safely, provided pineko will be adjusted...

@giacomomagni The point of this PR is precisely to break the runcard once more, since we know that there are leftovers of the December rework that are meaningless (rotations in the OperatorCard) or undesirable (_mu[2]grid).
Since we're going to break the runcards, I asked you to provide as many improvements as possible, but nothing came, so I just kept going with what I knew that had to be fixed.

So, please, @andreab1997, @giacomomagni, and @felixhekhorn provide feedback on the current status of runcards, such that I can make a proposal for the final layout (of course also @niclaurenti and @t7phy are invited to comment, but I expect the played much less with these runcards).

However, as mentioned this should break the producer interface, in the sense that existing runcards might have to be manually/automatically upgraded, but it should not break the consumer interface. I.e. if you computed an EKO, you should still be able to read it (inside eko.io.struct the mu2grid terminology was already used).

EDIT: the part about the consumer interface will be true up to the point in which we will actually use the specified number flavors in mugrid to implement the evolution part of "numerical FONLL": since now you access an operator with a scale, and after you will need a scale and a number of flavors, this will break compatibility (but it is a genuinely new feature: it is really difficult to add it without breaking anything...)

@alecandido
Copy link
Member Author

alecandido commented Mar 19, 2023

It's the running without the mixed terms (i.e. the QED corrections to the QCD beta function and the QCD corrections to the QED beta function). So in this case the two Qref can be different.

@niclaurenti If they are decoupled (that is possibly inconsistent with usage in DGLAP) you will be able to use two different CouplingsRef instances: one for the alphas running, and the other for alphaem running. You can set the reference value of the unused coupling to nan, and just do nothing for that.

If they are decoupled, but the reference values are given at the same scale, you can also share the same instance, and just shut down the mixed terms :)

@alecandido alecandido force-pushed the rearrange-runcards-metadata branch from c9e5ff6 to e3ec8ed Compare March 31, 2023 16:14
@alecandido alecandido merged commit b5deebe into master Mar 31, 2023
@alecandido alecandido deleted the rearrange-runcards-metadata branch March 31, 2023 16:41
@alecandido alecandido mentioned this pull request Apr 21, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
output Output format and management refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants