Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Polarized heavy coeffs #185

Merged
merged 18 commits into from May 23, 2023
Merged

Polarized heavy coeffs #185

merged 18 commits into from May 23, 2023

Conversation

adrianneschauss
Copy link
Collaborator

@adrianneschauss adrianneschauss commented Apr 7, 2023 •
edited

Polarized Heavy Coefficient Functions
about: Using Felix's LeProHQ code to implement the heavy cf's for the structure functions just like the unpolarized case along with tests

@adrianneschauss adrianneschauss self-assigned this Apr 7, 2023
@alecandido alecandido changed the base branch from master to polarized-coeffs April 7, 2023 15:52
@felixhekhorn felixhekhorn changed the title Polarized_heavy_coeffs Polarized heavy coeffs Apr 11, 2023
@felixhekhorn felixhekhorn added enhancement New feature or request physics physics features labels Apr 11, 2023
@adrianneschauss
Copy link
Collaborator Author

pre-commit.ci autofix

@Radonirinaunimi Radonirinaunimi self-requested a review April 12, 2023 12:37
@Radonirinaunimi
Copy link
Member

@adrianneschauss Thanks for having a start at the implementation! Could you please revert back fe6d952? It is difficult to keep track of what have actually changed with 169 files modified.

It is usually ideal to fix only the files that have been modified. The entire refactoring could be done in a separate PR.

@alecandido
Copy link
Member

We can apply that commit in a separate PR, or even directly on master.

Just run pre-commit run --all-files on master, and push it directly. We can then rebase this branch on the new master.

@felixhekhorn
Copy link
Contributor

We can apply that commit in a separate PR, or even directly on master.

Just run pre-commit run --all-files on master, and push it directly. We can then rebase this branch on the new master.

Someone volunteers for the counterpart of NNPDF/eko#243 here in yadism? 😇

@Radonirinaunimi
Copy link
Member

We can apply that commit in a separate PR, or even directly on master.
Just run pre-commit run --all-files on master, and push it directly. We can then rebase this branch on the new master.

Someone volunteers for the counterpart of NNPDF/eko#243 here in yadism? innocent

I can volunteer 😃 !

Radonirinaunimi
Radonirinaunimi reviewed Apr 16, 2023
View reviewed changes
Copy link
Member

@Radonirinaunimi Radonirinaunimi left a comment •
edited

Choose a reason for hiding this comment

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

Here are some first batches of comments.

Important: Please move all the tests to the tests folder.

src/yadism/coefficient_functions/heavy/g1_nc.py Outdated Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/g1_nc.py Outdated Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/g1_nc.py Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/g1_nc.py Outdated Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/g1_nc.py Outdated Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/g1_nc.py Outdated Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/g1_nc.py Outdated Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/gl_nc.py Outdated Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/g4_nc.py Show resolved Hide resolved
src/yadism/coefficient_functions/heavy/test_g1.py Outdated Show resolved Hide resolved
@giacomomagni giacomomagni changed the base branch from polarized-coeffs to apfelxx_bench April 26, 2023 12:52
Base automatically changed from apfelxx_bench to polarized-coeffs April 28, 2023 15:49
Radonirinaunimi
Radonirinaunimi requested changes May 9, 2023
View reviewed changes
Copy link
Member

@Radonirinaunimi Radonirinaunimi left a comment

Choose a reason for hiding this comment

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

Some additional comments!

In particular, the three tests should be combine into one single file called for example test_cfh_pol as there is no need them to be in separate files. This would also prevent duplicating the class MockCouplingConstants.

tests/yadism/cf/test_cfh_g1.py Outdated Show resolved Hide resolved
tests/yadism/cf/test_cfh_g1.py Outdated Show resolved Hide resolved
tests/yadism/cf/test_cfh_g1.py Outdated Show resolved Hide resolved
adrianneschauss and others added 2 commits May 14, 2023 11:09
@adrianneschauss @Radonirinaunimi
Update tests/yadism/cf/test_cfh_g1.py
52edee0 
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
@adrianneschauss
Tanjona comments resolved commit
d0a20fd
@Radonirinaunimi Radonirinaunimi marked this pull request as ready for review May 14, 2023 09:33
@Radonirinaunimi
Copy link
Member

This PR is also ready!

felixhekhorn
felixhekhorn approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@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 like the PR being simple - should be fine ... maybe merge the base branch

Base automatically changed from polarized-coeffs to master May 17, 2023 15:03
giacomomagni
giacomomagni reviewed May 17, 2023
View reviewed changes
Copy link
Collaborator

@giacomomagni giacomomagni 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 some comments to improve the tests. After that I'm good.

tests/yadism/cf/test_cfh_pol.py Outdated Show resolved Hide resolved
tests/yadism/cf/test_cfh_pol.py Outdated Show resolved Hide resolved
tests/yadism/cf/test_cfh_pol.py Outdated Show resolved Hide resolved
@Radonirinaunimi
Copy link
Member

Thanks @giacomomagni! I'm merging this now.

@Radonirinaunimi Radonirinaunimi merged commit 08d16fe into master May 23, 2023
3 of 4 checks passed
@Radonirinaunimi Radonirinaunimi deleted the polarized_heavy_coeffs branch May 23, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecandido alecandido alecandido left review comments

@Radonirinaunimi Radonirinaunimi Radonirinaunimi approved these changes

@felixhekhorn felixhekhorn felixhekhorn approved these changes

@giacomomagni giacomomagni giacomomagni approved these changes

Assignees

@adrianneschauss adrianneschauss

Labels
enhancement New feature or request physics physics features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@adrianneschauss @Radonirinaunimi @alecandido @felixhekhorn @giacomomagni