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.

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

Implement v1 file format [WIP] #299

Draft
wants to merge 286 commits into
base: master
Choose a base branch
from
Draft

Implement v1 file format [WIP] #299

wants to merge 286 commits into from

Conversation

t7phy
Copy link
Member

@t7phy t7phy commented Jul 12, 2024

Feature TODOs:

  • Generalize Channel to support arbitrarily many PIDs. This is implemented with commits f32902a, 2417c92 and 03f2dc6.
  • The structure Order should support a fragmentation scale. Started with commits 5e281ae and 02e48e9.
  • The CLI should support varying this new fragmentation scale. We need the following point variations: 1 (no scale variation), 3 (vary all scales with the same factor), 7 and 9 (assuming that there's no fragmentation scale), and 17 and 27 (assuming there's a fragmentation scale)
  • Extend the evolution code to take care of (at least) one, two and three convolutions with possibly one, two and three different EKOs
  • Finally, we need new subgrid types that support possibly arbitrarily many scales and momentum fractions and which support filling or are optimized for space. We should use the PackedArray struct that we wrote some time ago.
  • To fix the tests, we need import v0 PineAPPL grids and convert the subgrids into the new subgrid types.
  • Add machine-readable MC results and uncertainties to calculate interpolation error #270

Code TODOs:

  • Fill in all the TODO comments.
  • Change types of members of Order to u8
  • Generalize channel! to arbitrarily many PIDs
  • Support arbitrarily many scales and fix import of flexible-scale fastNLO tables
  • Test evolution for flexible-scale fastNLO tables
  • Simplify bin treatment: remove BinInfo, and have instead fill limits (1d limits that only concern Grid::fill) and bin limits (n-dimensional limits)
  • Add fragmentation scales to Grid::evolve_info
  • Make it possible to have different initial factorization and fragmentation scales in an FK-table
  • Rename lagrange_subgrid
  • Add support for static scale detection in previous subgrid type
  • Rename PackedQ1X2SubgridV1
  • Remove NodeValues
  • Remove Mu2
  • Implement D=3 for general_tensor_mul in evolution.rs
  • In Grid::merge check if grids are compatible with each other before merging
  • Check whether there are similar functions

CAPI TODO:

  • increase code coverage
  • unbreak GridOptFlags (see commit 54a59f3)
  • rename pineappl_channel_* to pineappl_channels_*?
  • check generated grids of new example programs
  • rename -v1.cpp suffix of example programs to .cpp and rename the previous example programs from .cpp to -deprecated.cpp. This should clarify that the use of v0 functions is deprecated (but still works!)

Fortran module TODO:

  • add new functions

Python API:

  • update to pyo3-0.22.5, this possible now since numpy-0.22.0 was released. Figure out what new features we can leverage from 0.22.x (see its CHANGELOG):
    • numpy-2 support (works out of the box)
    • declarative modules and submodules
    • Python 3.13
    • recheck whether we still need #![allow(unsafe_op_in_unsafe_fn)]
  • Update tutorials
  • raise minimum required Python version to 3.7 in pyproject.toml

@t7phy t7phy linked an issue Jul 12, 2024 that may be closed by this pull request
15 tasks
@cschwan cschwan linked an issue Jul 31, 2024 that may be closed by this pull request
5 tasks
@cschwan cschwan removed a link to an issue Jul 31, 2024
15 tasks
@cschwan
Copy link
Contributor

cschwan commented Jul 31, 2024

I believe the list shown in #118 is probably a bit too ambitious for this pull request. Realistically, we should implement all the items listed in #172.

@cschwan cschwan changed the title makes the necessary changes for v1 format [WIP] Implement v1 file format [WIP] Aug 6, 2024
@janw20
Copy link
Collaborator

janw20 commented Nov 15, 2024

Hi, I completed the Fortran interface. I made some (minor) design decisions that I would like your opinion on:

  • Since Kinematics is an enum with tuple-like variants, it is converted to a C struct holding a tag variable and a C union. Fortran doesn't support unions, so I use a Fortran derived type (pineappl_kinematics) that also has a tag variable, but only one integer variable for the index instead of the C union. I found this to be the cleanest solution, or do you have a better idea?
  • For some reason, Fortran cannot do arrays of procedure pointers, so I created a new derived type for the PDF and alpha_s functions, pineappl_xfx and pineappl_alphas, that each holds the procedure pointer as a member named proc. This is needed so that an array of PDF functions can be passed to pineappl_grid_convolve. For consistency, I also changed this in pineappl_grid_convolve_with_one and pineappl_grid_convolve_with_two. This goes against keeping backward compatibility with the 0.x of Pineappl versions, but until there is a solution to Investigate if we can compile/install the Fortran module #257, I figured that the user would make their own copy of pineappl.f90 anyway and use that, so backward compatibility is not that important in the Fortran case.

I also noticed two small bugs in the v1 C++ examples which I fixed in my first commit. Related to that: Is there a reason why pineappl_grid_new creates the kinematics in the order (Q², x₁, x₂), and not (x₁, x₂, Q²), as was always the order in, e.g., pineappl_grid_fill? I think this might have been the reason for the bug in examples/cpp/fill-grid-v1.cpp line 113

@Radonirinaunimi
Copy link
Member

Hi @janw20, thanks a lot for completing the Fortran interface!

Hi, I completed the Fortran interface. I made some (minor) design decisions that I would like your opinion on:

* Since `Kinematics` is an enum with tuple-like variants, it is converted to a C struct holding a tag variable and a C union. Fortran doesn't support unions, so I use a Fortran derived type (`pineappl_kinematics`) that also has a tag variable, but only one integer variable for the index instead of the C union. I found this to be the cleanest solution, or do you have a better idea?

* For some reason, Fortran cannot do arrays of procedure pointers, so I created a new derived type for the PDF and alpha_s functions, `pineappl_xfx` and `pineappl_alphas`, that each holds the procedure pointer as a member named `proc`. This is needed so that an array of PDF functions can be passed to `pineappl_grid_convolve`. For consistency, I also changed this in `pineappl_grid_convolve_with_one` and `pineappl_grid_convolve_with_two`. This goes against keeping backward compatibility with the 0.x of Pineappl versions, but until there is a solution to [Investigate if we can compile/install the Fortran module #257](https://github.com/NNPDF/pineappl/issues/257), I figured that the user would make their own copy of `pineappl.f90` anyway and use that, so backward compatibility is not that important in the Fortran case.

I had a look at it and it looks good. I don't think there is a cleanest a more robust to approach this apart from what is being done now (actually, this was how I also envisioned how it'd look like).

I also noticed two small bugs in the v1 C++ examples which I fixed in my first commit.

Thanks for spotting and fixing this bug! I had this under my radar before but somehow forgot to fix it (maybe I remembered to only do so on the Python API).

Related to that: Is there a reason why pineappl_grid_new creates the kinematics in the order (Q², x₁, x₂), and not (x₁, x₂, Q²), as was always the order in, e.g., pineappl_grid_fill? I think this might have been the reason for the bug in examples/cpp/fill-grid-v1.cpp line 113

I don't think there is a real reason why the kinematics have to be ordered in such a way. The restriction is coming from these lines:

for entry in channel.entry() {
// TODO: we assume `idx` to be ordered as scale, x1, x2
let fx_prod = cache.as_fx_prod(&entry.0, order.alphas, &idx);
lumi += fx_prod * entry.1;
}

which has a TODO, so likely this will change.

When leaving out the `_dp` in `pineappl_interp_tuples`, `applgrid::fx2` does not converge
@janw20
Copy link
Collaborator

janw20 commented Dec 2, 2024

Just for the record, the panicking of the Fortran v1 examples could be resolved by explicitly declaring the constants given to pineappl_interp_tuples as double precision (i.e., suffixing them with _dp):

pineappl_interp_tuples(1e2_dp, 1e8_dp, 40, 3, q2_reweight, q2_mapping, interpolation_meth), &
pineappl_interp_tuples(2e-7_dp, 1.0_dp, 50, 3, x_reweight, x_mapping, interpolation_meth), &
pineappl_interp_tuples(2e-7_dp, 1.0_dp, 50, 3, x_reweight, x_mapping, interpolation_meth) &

Without this, applgrid::fx2 didn't converge for higher y values, which resulted in a panic when reaching unreachable!().

@Radonirinaunimi
Copy link
Member

Radonirinaunimi commented Dec 2, 2024

Thanks again @janw20 for fixing this issue! I still don't understand why this was only (?) happening on Arch, but I guess this doesn't matter anymore as the solution works perfectly.

@cschwan
Copy link
Contributor

cschwan commented Dec 17, 2024

@Radonirinaunimi and @janw20: could one of you take care of the new item "check generated grids of new example programs"? I believe I once did that unsuccessfully, so possibly there's a bug somewhere. The easiest way would be to always use the v1 grid in the remaining C++ examples. I believe that right now the generated grid isn't checked anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants