Skip to content

Polarized ToyLH from genpdf#225

Merged
giacomomagni merged 5 commits into
masterfrom
toylh_polarized
Mar 14, 2023
Merged

Polarized ToyLH from genpdf#225
giacomomagni merged 5 commits into
masterfrom
toylh_polarized

Conversation

@giacomomagni
Copy link
Copy Markdown
Collaborator

@giacomomagni giacomomagni commented Mar 14, 2023

This PR is to allow generation of a Polarised ToyLH pdf as lhapdf.PDF object.

@Radonirinaunimi to generate it you can do:

genpdf generate ToyLH_polarized 21 1 2 3 4 -p toy_pol -i

@felixhekhorn felixhekhorn added enhancement New feature or request benchmarks Benchmark (or infrastructure) related labels Mar 14, 2023
@felixhekhorn
Copy link
Copy Markdown
Collaborator

In a future PR to address #122 we should defer the from banana import toy from top level to that specific elif (of course likewise for the unpolarized case) (or should we do it here?)

@felixhekhorn
Copy link
Copy Markdown
Collaborator

@t7phy in case you add your toy function inside banana (as we discussed yesterday) you maybe also want to mirror this PR in the similar way

@giacomomagni
Copy link
Copy Markdown
Collaborator Author

giacomomagni commented Mar 14, 2023

In a future PR to address #122 we should defer the from banana import toy from top level to that specific elif (of course likewise for the unpolarized case) (or should we do it here?)

okay fine, seems doable without any drawback.

@giacomomagni
Copy link
Copy Markdown
Collaborator Author

@t7phy in case you add your toy function inside banana (as we discussed yesterday) you maybe also want to mirror this PR in the similar way

yes let's fix also that. I can't see any commit banana though.

@giacomomagni giacomomagni linked an issue Mar 14, 2023 that may be closed by this pull request
@felixhekhorn
Copy link
Copy Markdown
Collaborator

@t7phy in case you add your toy function inside banana (as we discussed yesterday) you maybe also want to mirror this PR in the similar way

yes let's fix also that. I can't see any commit banana though.

just to repeat again a piece of private conversation: @t7phy is going for LHAPDF-like benchmarks first

Copy link
Copy Markdown
Collaborator

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

I'm happy with the change, but are you sure this will close #122 completely? (i.e. there is no other dependency?)

@giacomomagni
Copy link
Copy Markdown
Collaborator Author

I'm happy with the change, but are you sure this will close #122 completely? (i.e. there is no other dependency?)

All the other occurrences of banana are in ekomark

Comment thread src/ekobox/genpdf/__init__.py Outdated
Comment thread src/ekobox/genpdf/__init__.py Outdated
@alecandido
Copy link
Copy Markdown
Collaborator

All the other occurrences of banana are in ekomark

Not sure this solves the issue, unfortunately, since ekobox depends on ekomark.

However, I'm fine with this change, if it helps anyhow.

@giacomomagni
Copy link
Copy Markdown
Collaborator Author

Not sure this solves the issue, unfortunately, since ekobox depends on ekomark.

True, there is a single point where ekomark is used which is here, so we have to leave the issue open.

@felixhekhorn
Copy link
Copy Markdown
Collaborator

Not sure this solves the issue, unfortunately, since ekobox depends on ekomark.

True, there is a single point where ekomark is used which is here, so we have to leave the issue open.

if that is true, we can/should move that module to ekobox (can/should be done in a separate PR) - also content-wise it is easy to argue that this module should belong to ekobox: it is a user interface and in fact has nothing to do with benchmarking

@felixhekhorn
Copy link
Copy Markdown
Collaborator

it is a user interface and in fact has nothing to do with benchmarking

just to be fair: it is used inside ekomark, because logs are on PDFs - this way we would invert the dependency: ekomark then depends on ekobox, which maybe is more acceptable

@giacomomagni
Copy link
Copy Markdown
Collaborator Author

giacomomagni commented Mar 14, 2023

it is a user interface and in fact has nothing to do with benchmarking

just to be fair: it is used inside ekomark, because logs are on PDFs - this way we would invert the dependency: ekomark then depends on ekobox, which maybe is more acceptable

Okay let's do one step at the time and address this in another PR, since that is out of the scope of this one.
If there are no other problems I'll merge by the end of the afternoon.

@giacomomagni giacomomagni merged commit a9813dc into master Mar 14, 2023
@felixhekhorn felixhekhorn deleted the toylh_polarized branch March 14, 2023 16:35
@alecandido
Copy link
Copy Markdown
Collaborator

alecandido commented Mar 14, 2023

if that is true, we can/should move that module to ekobox (can/should be done in a separate PR) - also content-wise it is easy to argue that this module should belong to ekobox: it is a user interface and in fact has nothing to do with benchmarking

@felixhekhorn Just notice that that function was originally in eko, then in ekobox, then in ekomark, because we argued that it didn't make sense that the internal module depended on the user-facing one.
Well, it now makes sense. Fine by me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarks Benchmark (or infrastructure) related enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants