Refactor msr #1781

APJansen · 2023-07-18T13:54:10Z

Adds a regression test for the MSR_Normalization layer.
Mainly improving readability by rewriting the indices.
Also makes it compatible with higher rank pdfs by changing a flatten(y) to y[0] and allowing for an out_shape rather than only a 1d output_dim.

Awaiting further tests as per discussion here.

RoyStegeman

Not completely done yet with my review, but I ran out of time. Anyway, here are a few comments

n3fit/src/n3fit/layers/msr_normalization.py

n3fit/src/n3fit/msr.py

n3fit/src/n3fit/layers/msr_normalization.py

RoyStegeman · 2023-07-27T09:26:10Z

n3fit/src/n3fit/layers/msr_normalization.py

+IDX = {
+    'photon': 0,
+    'sigma': 1,
+    'g': 2,
+    'v': 3,
+    'v3': 4,
+    'v8': 5,
+    'v15': 6,
+    'v35': 7,
+    'v24': 8,
+}


It doesn't affect the result, but the order of v35 and v24 is reversed.

Since we rely on hardcoding the basis of the fktable anyway, it may be best to have a dict like this one stored in a single location which all modules that need it can then import (which at the moment I think are just x_operations.py, the current module and writer.py).

I agree, do you want me to do that in this commit? If so I'm not sure where to put it.

That's also what I'm not too sure about, perhaps we should add an n3fit/utils.py? @scarlehoff do you have any ideas?

The information should be here

nnpdf/validphys2/src/validphys/pdfbases.py

Line 695 in 9cf5f9e

def fitbasis_to_NN31IC(flav_info, fitbasis):

incidentally, the name of that function, fitbasis_to_NN31IC, is no longer correct.

Some things that we said (a very long time ago) someone should do are:

Create a function that generate the PDF output -> fktable rotation and use that on the fit (half of the work @APJansen already did with Implement FkRotation as subclass of Rotation by rewriting using a rotation tensor #1772)

Define in that module what the "final fitbasis" actually is so that both fitbasis_to_NN31IC and any other function can use that information.

Maybe fitbasis_to_fitoutput would be more correct. Or even skip that step and have just fitbasis_to_fkbasis since the fitoutput->fkbasis step is known and constant. That would also require (small) changes to the QED fit or the integration.

This is probably better left to another PR?

scarlehoff

I wrote a lot because I was writing as I went but basically the summary is:

Feel free to change the whole msr_normalization.py layer (as long as the input-outputs are the same) and the more things you can put as constants at the top of the module the better.
I'd strongly prefer to leave the input fitted (integrated) PDF and photon separated.

n3fit/src/n3fit/layers/msr_normalization.py

scarlehoff · 2023-07-28T13:21:40Z

n3fit/src/n3fit/msr.py

-    normalization_factor = MSR_Normalization(output_dim, mode, name="msr_weights")(
-        pdf_integrated, photon_integral
-    )
+    normalization_factor = MSR_Normalization(mode, name="msr_weights")(pdf_integrated)


Why this change? I feel the previous one was actually much clearer since it makes the difference between the PDF integration and the photon explicit in the MSR.

(and they are really different objects, while the PDF comes from the NN, the photon comes from an analytical computation and is constant throughout the fit, so the separation in the call is good)

Maybe I misunderstand (I never understood why the photon can't be treated the same) but they're not really different objects right? They might arise differently but they are all integrals right? I thought a dedicated layer that inserts the photon integral in the first component makes it more transparent, and simplifies the MSR layer.
But if you still prefer the old way I'll change it back.

The photon is analytically computed before the fit
https://github.com/NNPDF/nnpdf/blob/9cf5f9ecc83ec59fff804f859b531198139021fe/validphys2/src/validphys/photon/compute.py#L125C13-L125C13

Computing the integral in the same way of the others would require too many points and the photon contribution is not large so we don't need the same level of precision.

So, while they are all "the result of integrals", the photon is a constant that gets injected into the calculation while the others are quantities that are computed and change from epoch to epoch.

Ok, I changed it back to what it was.

Any idea why the CI is failing? Thought it was some fluke but it happend twice in a row now, just says the operation was cancelled when solving the environment..

github-actions · 2023-07-28T16:20:02Z

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Fit Name: NNBOT-ab3804063-2023-07-28
Fit Report: https://vp.nnpdf.science/OTyi0BneSSWrLjOPATy7Sg==
Fit Data: https://data.nnpdf.science/fits/NNBOT-ab3804063-2023-07-28.tar.gz

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

APJansen · 2023-08-08T15:29:48Z

@scarlehoff @RoyStegeman Tests are failing, at the stage of resolving the environment. Also on master now after merging the preprocessing and rotation PRs, though I assume only because they reran and something external has happened, I can't imagine why the code changes would break the conda environment when I didn't add or remove any dependencies. Not sure what to do about this?

scarlehoff · 2023-08-08T15:51:49Z

The problem with macos seems to be different (and real, but stochastic?)
linux / python 3.9 seems to be something wrong with conda, not sure what triggered it but has been there for a while and in many branches (e.g. #1785)

(this is a time as good as any other to promote #1773)

scarlehoff · 2023-08-10T08:48:12Z

I see the CI has come back to life (yey!)

Is this ready for review? (I'm going to be on holiday starting next week and I'd like to review the changes because of being a control-freak but also don't want to block the progress in the GPU/multireplica stuff ¡!)

APJansen · 2023-08-10T12:13:39Z

Ah great :) Yes it's ready. You wouldn't really block progress, but if it's possible to review before you go that'd be great :)

I'll reply to #1773 there

scarlehoff

Beyond the stylstic nitpicking, I think this is good to go.

Ah great :) Yes it's ready. You wouldn't really block progress, but if it's possible to review before you go that'd be great :)

Better safe than sorry!

n3fit/src/n3fit/layers/msr_normalization.py

scarlehoff · 2023-08-10T12:29:51Z

n3fit/src/n3fit/layers/msr_normalization.py


        Returns
        -------
        normalization_factor: Tensor(14)
            The normalization factors per flavour.
        """
-        y = op.flatten(pdf_integrated)
+        y = pdf_integrated[0]  # get rid of the batch dimension
+        photon_integral = photon_integral[0]  # get rid of the batch dimension


Does the photon integral need a batch dimension? You could as well remove the [[ in below (or is it because of symmetry arguments?)

(you can also do directly here photon_integral[0][0] btw)

I suppose this was more relevant when I actually joined them together before being passed to this layer, but still having the same shapes makes it easier to change things I think, even if it's a bit superfluous now.

n3fit/src/n3fit/layers/msr_normalization.py

Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>

APJansen · 2023-08-11T09:09:00Z

Conda broke again... I'll rerun the tests once in a while and merge it if it passes, thanks for the review :)

APJansen added 9 commits July 13, 2023 14:51

Add test of MSR_Normalization layer

cc57f4a

Add photon integral before MSR_Normalization layer

fa5333f

Simplify indices

ab4bb63

Correct shape in docstring

83c1f65

Change notation for indices

d1482c8

Replace flatten with [0] to get rid of batch dimension

073df14

Change output_dim to automatically inferred out_shape

36c2fff

Fix bug with photon

50fb75a

Rename test_layer so its not seen as a test

c1dd925

APJansen added easy Refactoring n3fit Issues and PRs related to n3fit labels Jul 18, 2023

APJansen requested review from Radonirinaunimi and RoyStegeman July 18, 2023 13:54

This was referenced Jul 18, 2023

Refactor rotations #1780

Merged

Multi Replica PDF #1782

Closed

RoyStegeman reviewed Jul 27, 2023

View reviewed changes

APJansen added 4 commits July 27, 2023 13:25

Remove unused output_dim argument and out_shape attribute

a25bb57

Fix order of v24, v35

ac8be51

Merge branch 'master' into refactor_msr

800a663

Fix typo on out_shape

db2ac8b

scarlehoff added the run-fit-bot Starts fit bot from a PR. label Jul 28, 2023

scarlehoff reviewed Jul 28, 2023

View reviewed changes

APJansen added 5 commits August 7, 2023 14:32

Define MSR and VSR indices as global constants

8dc3814

Remove build method

a418e99

Add/improve documentation

9685827

factor out definitions of normalization to top as global constants

bc41e61

Undo joining of photon integral

eb73f5a

turn lists into tensors

96b798b

scarlehoff approved these changes Aug 10, 2023

View reviewed changes

APJansen and others added 3 commits August 10, 2023 16:30

Add module docstring

b52a6f4

Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>

Avoid duplicate names

09a99b8

formatting

a85d314

Merge branch 'master' into refactor_msr

d8d677e

APJansen merged commit cfcb577 into master Aug 18, 2023

APJansen deleted the refactor_msr branch August 18, 2023 11:26

APJansen mentioned this pull request Dec 4, 2023

Multi Replica PDF #1880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor msr #1781

Refactor msr #1781

APJansen commented Jul 18, 2023

RoyStegeman left a comment

RoyStegeman Jul 27, 2023

APJansen Jul 27, 2023

RoyStegeman Jul 27, 2023

scarlehoff Jul 28, 2023

APJansen Aug 8, 2023

scarlehoff left a comment

scarlehoff Jul 28, 2023

APJansen Aug 7, 2023

scarlehoff Aug 7, 2023

APJansen Aug 8, 2023

github-actions bot commented Jul 28, 2023

APJansen commented Aug 8, 2023

scarlehoff commented Aug 8, 2023

scarlehoff commented Aug 10, 2023

APJansen commented Aug 10, 2023

scarlehoff left a comment

scarlehoff Aug 10, 2023

APJansen Aug 11, 2023

APJansen commented Aug 11, 2023

Refactor msr #1781

Refactor msr #1781

Conversation

APJansen commented Jul 18, 2023

RoyStegeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scarlehoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 28, 2023

APJansen commented Aug 8, 2023

scarlehoff commented Aug 8, 2023

scarlehoff commented Aug 10, 2023

APJansen commented Aug 10, 2023

scarlehoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APJansen commented Aug 11, 2023