Conversation
scarlehoff
left a comment
There was a problem hiding this comment.
The changes are basically what we want.
I only have the two comments I added: a test would be nice and a more robust way of saying "this is the evolution basis".
|
|
||
|
|
||
| def rotation(flav_info): | ||
| """Returnd a rotation matrix which takes from the flavour to the evolution basis, |
|
|
||
| def call(self, x_raw): | ||
| return self.tensor_product(x_raw, self.rotation_matrix, 1) | ||
|
|
There was a problem hiding this comment.
We also need a test for this layer in n3fit/tests
Something like computing the rotation in numpy and ensuring the layer produces the same thing.
There was a problem hiding this comment.
I ve added a check for this. Hopefully now it s fine. As I was saying I think a transposition was missing, I ve added it at the level o pdfbases.py, and I ve expanded a bit the docstring to avoid confusion with the indices
| mat.append(f[flav_name]) | ||
| # if one of the keys in the dictionary is not a key in flist | ||
| # it means we are already in the evolution basis | ||
| except KeyError: |
There was a problem hiding this comment.
Not sure about this, maybe the flav_info should be the one saying whether we are in the evolution basis or not. Otherwise if the user writes "udar" by mistake it would be understood as evolution basis and will produce wrong results.
That said, that the information in the flavour info dictionary is valid should be checked by validphys before going inside n3fit.
There was a problem hiding this comment.
I guess we can either implement some kind of check in vp2, maybe in a separate, or to add an explicit flag somwhere
There was a problem hiding this comment.
A check on the input would be needed either way. In the sense that if the user says they are using "evol" the entries in the dictionary should really correspond to that.
There was a problem hiding this comment.
I ve tried to add a check on the input when defining performfit. It looks as if it does the job, but I have no idea if this is the way of doing it
|
Ehm..we may have a bug. Assuming that in the runcard the flavour are given with this ordering then the matrix which brings these vector to is In with |
|
I think this bug was introduced when replacing with in the call function of FlavourToEvolution. The two things are not equivalent if we don t transpose the matrix I think. Sorry, will check again, fix it and run again some test |
The tensorproduct with axes=1 is basically a dot product between argument a and b so the first argument in the matrix should be the "flavour index" and the second the "evolution index". In any case, this is exactly why a test where the rotation is done in numpy and then compared to the answer of this layer is important :P |
|
yes. The first index (the line) of the matrix above refers to the evolution index, while the second index refers to the flavour index, so we need to transpose it.
yep |
| for flav_dict in fitting['basis']: | ||
| flav_name = flav_dict['fl'] | ||
| if flav_name not in allowed_labels: | ||
| raise CheckError(f"{flav_name} is not a valid flavour name") |
There was a problem hiding this comment.
I guess @Zaharid should chime in on right way of checking for input. I think I'd do something similar.
The only thing I'd add is a check to ensure that no flavour is repeated.
As a desire, I'd take also as input the name of the basis and check that the allowed labels are the ones that correspond to the chosen basis.
There was a problem hiding this comment.
This could indeed use the stuff we already have, and implement a check similar to
|
I ve run a test with the code from this branch after fixing the transposition of the rotation matrix. |
|
@scarlehoff @Zaharid are you happy with this PR? |
|
Let me go through it (and push a minor change to the |
scarlehoff
left a comment
There was a problem hiding this comment.
I'm happy with the PR, the only comment is the one about the check. We can use the existing functionality (that way we ensure the basis used in n3fit will always be the ones defined in the vp pdbases)
|
I m a bit confused about the reason the checks are failing..they pass on mac but not on linux, but if I try to run on my laptop the test which is failing I don t have any problem |
|
@Zaharid, any reasons not to use In any case, the situation seems to stem from here nnpdf/validphys2/src/validphys/calcutils.py Line 179 in 4bd456d -1e-17 so 1/almost_0- is almost -inf and the condition is True.
|
Here I ve cherry-picked the commits regarding the implementation of the flavour basis in n3fit.
Should be ready for merging.