Skip to content

Add photon#1643

Merged
scarlehoff merged 226 commits into
masterfrom
add-photon
May 19, 2023
Merged

Add photon#1643
scarlehoff merged 226 commits into
masterfrom
add-photon

Conversation

@niclaurenti
Copy link
Copy Markdown
Contributor

@niclaurenti niclaurenti commented Dec 2, 2022

  • Do a fit with Theory 133 (3.1)
  • Compare with a fit with Theory 53 (3.1)
  • Add tests (and fix tests) to this PR
  • add fiatlux to conda recipe
  • add eko to conda recipe
  • restore top-level imports
  • add documentation
  • (optional) make fiatlux accept dict as input
  • Run black and isort using pyproject.toml from master

@RoyStegeman
Copy link
Copy Markdown
Member

Should this already be reviewed (after fixing the comput_photon path)?

@niclaurenti
Copy link
Copy Markdown
Contributor Author

Should this already be reviewed (after fixing the comput_photon path)?

Hi @RoyStegeman, no it is not ready. i opened the pull request so that me @alecandido @scarlehoff and whoever is interested can discuss on its implementation.

@RoyStegeman
Copy link
Copy Markdown
Member

That's what I assumed, thanks

@scarlehoff scarlehoff marked this pull request as draft December 2, 2022 17:18
Comment thread n3fit/src/n3fit/layers/rotations.py Outdated
Comment thread n3fit/src/n3fit/layers/rotations.py Outdated
Comment thread n3fit/src/n3fit/model_gen.py Outdated
Comment thread n3fit/src/n3fit/model_trainer.py Outdated
Comment thread n3fit/src/n3fit/performfit.py Outdated
Comment thread n3fit/src/n3fit/scripts/n3fit_exec.py Outdated
@scarlehoff
Copy link
Copy Markdown
Member

Have a look at #1644 and rebase this branch on top of that one (or even copy the changes you want on top of that one). In that one you have the xgrid before creating the model.

@alecandido alecandido changed the base branch from master to separate_xgrid_from_model December 5, 2022 12:33
Comment thread n3fit/src/n3fit/layers/rotations.py Outdated
Base automatically changed from separate_xgrid_from_model to master December 7, 2022 15:29
Copy link
Copy Markdown
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

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

I left a few comments here and there. In general I would recommend to keep your code a bit cleaner, even if the intention is just to prototype instead of creating a final product. That helps us since it's easier to read and yourself in the end as well.

Comment thread n3fit/src/n3fit/model_trainer.py Outdated
Comment thread n3fit/src/n3fit/model_trainer.py Outdated
Comment thread validphys2/src/validphys/photon_pdf/compute_photon.py Outdated
Comment thread n3fit/src/n3fit/layers/rotations.py Outdated
Comment thread n3fit/src/n3fit/model_gen.py
Comment thread n3fit/src/n3fit/layers/rotations.py Outdated
@RoyStegeman
Copy link
Copy Markdown
Member

But yes on my part it's fine to run black and isort on all files, I think I've asked enough for one PR ;)

@scarlehoff
Copy link
Copy Markdown
Member

Did you look at the tests @scarlehoff?

Since we agreed that the tests will be revisited in the future with a "theory 398" with a smallish photon I didn't double down on them but I stand by my previous review of those.

Let me link here two comments that should be dealt with:

But I think it would be easier (and less overwhelming for @niclaurenti, this is already gigantic as it is) to do that after merging the code.

it would be good to add docs

It would. Maybe a few lines here https://docs.nnpdf.science/n3fit/runcard_detailed.html detailing the fiatlux dictionary would be nice.

Comment thread doc/sphinx/source/n3fit/runcard_detailed.rst Outdated
@RoyStegeman
Copy link
Copy Markdown
Member

Maybe a few lines here https://docs.nnpdf.science/n3fit/runcard_detailed.html

For symmetry reasons with MHOU fits I think it makes more sense to put a section "how to do a qed fit" under "tutorials"

@RoyStegeman
Copy link
Copy Markdown
Member

@niclaurenti I think you ran black and isort using the default settings instead of those in pyproject.toml https://github.com/NNPDF/nnpdf/blob/master/pyproject.toml.

Please revert the last commit and rerun with those settings

Comment thread doc/sphinx/source/tutorials/run-fit.md
@github-actions
Copy link
Copy Markdown

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

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

Comment thread n3fit/src/n3fit/scripts/n3fit_exec.py Outdated
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label May 17, 2023
@scarlehoff
Copy link
Copy Markdown
Member

It seems that you forgot to update the test in the max_q2 commit. Once that is done we can merge this.

(I've moved the point about theory 398 to this issue #1735. Since I'll have more time next week I'll see about creating the small ekos to test the lhapdf info files as well)

@andreab1997
Copy link
Copy Markdown
Contributor

@niclaurenti please fix also this other evolven3fit_new problem

log.setFormatter(logging.Formatter(LOGGING_SETTINGS["formatter"]))

Here you just need to remove logging.Formatter(

Comment thread doc/sphinx/source/tutorials/run-qed-fit.rst Outdated
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
@scarlehoff scarlehoff merged commit c7bb77b into master May 19, 2023
@scarlehoff scarlehoff deleted the add-photon branch May 19, 2023 12:01
@RoyStegeman RoyStegeman mentioned this pull request Jun 22, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants