Skip to content

Conversation

@evagroenendijk
Copy link
Contributor

No description provided.

@evagroenendijk evagroenendijk linked an issue Jan 8, 2026 that may be closed by this pull request
@felixhekhorn
Copy link
Contributor

Can we please fix this deeper?

  • remove q_fin from construct_eko_photon_cards - it is not used there: see here
  • drop q_fin and q_points from the CLI

Can we please add a unit test? I'm surprised this has not come up earlier, because I suspect @jacoterh found the bug by just running the real life thing

@evagroenendijk
Copy link
Contributor Author

Can we please fix this deeper?

  • remove q_fin from construct_eko_photon_cards - it is not used there: see here
  • drop q_fin and q_points from the CLI

Can we please add a unit test? I'm surprised this has not come up earlier, because I suspect @jacoterh found the bug by just running the real life thing

@felixhekhorn thanks for the comment, I completely agree! I was indeed looking at your first point, the second I’ll try later.

About the unit test, I tried to add something like it before, but did not add it eventually because it takes a long time even with few x points at LO (it has to produce a whole eko). Do we want to run that every time, or are you thinking of something else?

@scarlehoff
Copy link
Member

I'm surprised this has not come up earlier,

Indeed.

Can we please add a unit test?

I think Andrea also tried and it was way too slow. We might want to have some tests that are run only before merging to master though, so that things show up much earlier.

0.06445395993068814,
0.03255362482741475
0.03370335418731024,
0.0337033541873098,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scarlehoff Can I ask something? I regenerated these quickcards because they were not passing the regression tests anymore. But to be completely honest I don't understand 100% what these values are. The value that gave the regression error was the second of the integrability ones.

Now there is no problem anymore, but I wanted to check if I did not do it wrong

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The integrability numbers is basically the PDF picked up at a very low x so they can easily vary depending on the computer the tests are run.

I'm guessing that they varied in your computer (which is a mac?) so you might need to revert these changes here.

Btw, I think you will need to rebase on top of master (it should be painless, you can probably do it directly from github) for all the tests to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks a lot! Yes, my computer is a mac. Each time it was exactly the same value that differed though, is that not a bit suspicious?

And I am just testing the eko production on the cluster now, once it's done I'll rebase etc!

Copy link
Member

Choose a reason for hiding this comment

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

No, I think if we were to do this test on a mac we would probably find always the same value (and what you got). That's why this test runs only in Ubuntu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I didn't know that they only ran in ubuntu, thanks! I'll revert these changes then

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.

Bug in evolven3fit introduced in 7d768c9

4 participants