Atlas do not infer correct values from np.array

[giacomomagni]

While trying to use the new eko 0.13.1 inside pineko I found this weird behaviour,
which I'm not sure we are aware of.

>>> from eko.matchings import Atlas, nf_default
>>> import numpy as np
>>> atlas = Atlas([1,2,3], (2,4))
>>> atlas.walls
[0, 1, 2, 3, inf]
>>> atlas1 = Atlas(np.array([1,2,3]), (2,4))
>>> atlas1.walls
array([inf, inf, inf])
>>> nf_default(2.5, atlas)
5
>>> nf_default(2.5, atlas1)
2
>>> 

Do you know if this is might happen also for other classes such as Couplings ?

[alecandido]

eko/src/eko/matchings.py

Line 63 in 0994336

def __init__(self, matching_scales: MatchingScales, origin: EPoint):

eko/src/eko/quantities/heavy_quarks.py

Line 65 in 0994336

MatchingScales = HeavyQuarks[MatchingScale]

eko/src/eko/quantities/heavy_quarks.py

Line 22 in 0994336

class HeavyQuarks(list, Generic[T]):

So matching_scales has to be a list, and the editor should warn you if you're using something else.

The error is coming from:

eko/src/eko/matchings.py

Line 65 in 0994336

self.walls = [0] + matching_scales + [np.inf]

since I'm using list concatenation, but I specified that I want a list.

So, instead of using runtime checks, adding repeated code for it over and over, and consequently (an irrelevant) runtime overhead, I decided to rely on type-hints.
As soon as possible, I will set pre-commit (if not too complex) and the CI to raise errors if the type check fails.

[giacomomagni]

I see, thanks for your precise reply.

So far I see this was unspotted in pineko
let me try to fix tests or benchmark to see if we were able to find it out.

For my understanding why we can't use:

 self.walls = [0, *matching_scales, np.inf] 

which should work both for list and np.array ?

[alecandido]

For my understanding why we can't use:

We can, there is no problem. But the usage of NumPy arrays will be officially unsupported. So if it works, good for you, but if it will break, that will be simply the consequence of exploiting undefined behavior.

There are two elements involved:

• supporting a broad input, doing checks and conversions, against ask for the exact input you need: I'm trying to stick to the second strategy, as said above, in order to reduce code complexity, and following principles in the beloved PEP20
  • this is disputable: you may argue that you prefer a broader set of inputs, but this will bring a lot of ifs, pathlib.Paths, np.arrays, and .tolist()s, when you mostly do not need them, if you are just consistent with a single format
• public API: I'm declaring I want a MatchingScales type, so that one is the only supported input; then Python is dynamic, and it will run with everything, but you're warned that it might crash or even returning a different result

[felixhekhorn]

Do you know if this is might happen also for other classes such as Couplings ?

@giacomomagni For the specific case we're consistent

eko/src/eko/couplings.py

Lines 454 to 456 in 0994336

	matching_scales = (np.array(masses) * np.array(thresholds_ratios)).tolist()
	self.thresholds_ratios = list(thresholds_ratios)
	self.atlas = matchings.Atlas(matching_scales, (couplings.scale**2, nf_ref))

I hope we're elsewhere as well ...

As soon as possible, I will set pre-commit

@alecandido I guess you want https://github.com/pre-commit/mirrors-mypy (see also #178 )

[giacomomagni]

Do you know if this is might happen also for other classes such as Couplings ?

@giacomomagni For the specific case we're consistent

I was referring also to some cases like here, where I was not sure:

https://github.com/NNPDF/pineko/blob/6bb08cbc9c83130f870d8a11779ff6633e1459fe/src/pineko/evolve.py#L194

I hope we're elsewhere as well ...

Let's hope so 😬

In any case since I understand this is a desired behaviour, feel free to close the issue.

[felixhekhorn]

I can see the point of @alecandido , but I also agree partially with @andreab1997 : numpy is an integral part of EKO (we can not work without), but we don't like it in the input - this is not very obvious to an outsider ... for the case given by @giacomomagni above this can lead to (np.power([2,4.75,175],2).tolist()

public API: I'm declaring I want a MatchingScales type, so that one is the only supported input; then Python is dynamic, and it will run with everything, but you're warned that it might crash or even returning a different result

@andreab1997 was arguing along this line: you're not warned, not visibly

[andreab1997]

I can see the point of @alecandido , but I also agree partially with @andreab1997 : numpy is an integral part of EKO (we can not work without), but we don't like it in the input - this is not very obvious to an outsider ... for the case given by @giacomomagni above this can lead to (np.power([2,4.75,175],2).tolist()

public API: I'm declaring I want a MatchingScales type, so that one is the only supported input; then Python is dynamic, and it will run with everything, but you're warned that it might crash or even returning a different result

@andreab1997 was arguing along this line: you're not warned, not visibly

Indeed. I would say that if also developers of eko like @felixhekhorn and @giacomomagni did not notice this bug in pineko (which we noticed later with @felixhekhorn), it means that it really is confusing and that there is no hope for an external user to notice

[giacomomagni]

Indeed I found this just looking by eyes at the runcards,
but on the other side I am more reassured that pineko tests were able to find it out.
I feel this is something that should benefit of having good change log #216, since
most likely can affect also external projects like nnusf, dis_tp and other simple scripts for which
we might not have good tests.

[alecandido]

Indeed. I would say that if also developers of eko like @felixhekhorn and @giacomomagni did not notice this bug in pineko (which we noticed later with @felixhekhorn), it means that it really is confusing and that there is no hope for an external user to notice

At the moment, Atlas itself is not part of the public API, so external users are not supposed to use it (unfortunately, this makes Pineko an internal user, making use of EKO internals... but that's what it is, until stabilization).

The idea is that, the moment it will be, the "warning" will be the docs. And actually is already there:
https://eko.readthedocs.io/en/latest/modules/eko/eko.html#eko.matchings.Atlas
NumPy and other popular libraries are doing the same: they just document their API. So, the only way to avoid bugs would be reading the docs (simply we do not have to add the type to the docstring manually, since it is extracted from the type hint).
And that's what is done inside EKO.

The only problem is that EKO is documenting everything, so even the internals are documented at the same level of the public API. That is good for EKO developers, but we should split the public API from the rest (the hope was that introducing __all__ attributes will help us in this).

Then for sure we'd need a changelog. But hopefully we will need it less, because with the public API we will also try to enforce a certain stability.

@andreab1997 was arguing along this line: you're not warned, not visibly

For this, we are not using any solution for NumPy or any other external library (other than reading docs): if you use it wrongly, it can give wrong results (or crash, if you're lucky).

However, the new adoption of type hints would unlock even that possibility, that is something more, not less. And you proposed yourself: we can add mypy to pre-commit, and I would add it also to the testing workflow, right before or right after PyLint.