Skip to content

Update Python docs #301

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

Merged
merged 7 commits into from
Aug 9, 2024
Merged

Update Python docs #301

merged 7 commits into from
Aug 9, 2024

Conversation

felixhekhorn
Copy link
Contributor

@felixhekhorn felixhekhorn commented Jul 24, 2024

Closes #157

This mostly updates and fixes all Python doc strings.

Apart from that there are few more (even breaking) changes:

  • PyGrid::evolve got deprecated (as the underlying function)
  • PyFkTable::convolve_with_two: return type was harmonized with PyFkTable::convolve_with_one to avoid the deprecation of into_pyarray
  • the transition "Lumi" -> "Channel" was performed both on the PyO3 and Python side

if necessary we could also add other changes to the Python interface here, such as to expose Grid::convolutions

@felixhekhorn felixhekhorn added the documentation Improvements or additions to documentation label Jul 24, 2024
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.

Thanks for this @felixhekhorn.

I will have a detailed look as soon as possible but I've picked up very tiny typos while I was skimming through.

Are you also planning to modify the occurrences of lumi(_) in the argument of the functions and in the variables names of src/fk_table.rs, src/grid.rs, and pineappl/grid.py?

@cschwan
Copy link
Contributor

cschwan commented Aug 2, 2024

#157 is a related Issue.

@felixhekhorn
Copy link
Contributor Author

Are you also planning to modify the occurrences of lumi(_) in the argument of the functions and in the variables names of src/fk_table.rs, src/grid.rs, and pineappl/grid.py?

I hope I caught all remaining occurrences - the only remaining appearance of the word "lumi" in pineappl_py/src is related to pineappl::convolutions::LumiCache, which I did not touch (as the underlying Rust is not touching it).

There is some overlap with #302 since both are touching the files in pineappl_py/src

@cschwan
Copy link
Contributor

cschwan commented Aug 7, 2024

I ran cargo fmt in commit fd6f422. @felixhekhorn, could you check that these changes don't break anything?

@cschwan
Copy link
Contributor

cschwan commented Aug 7, 2024

I can merge this now if you like, for wrappers of Grid::{convolutions,set_convolutions} we should open a new PR.

When this will be merged into master, I'm going to release this as a new minor version (0.9.0) instead of increasing the patch version of 0.8, because we renamed publicly-visible items.

@felixhekhorn
Copy link
Contributor Author

I ran cargo fmt in commit fd6f422. @felixhekhorn, could you check that these changes don't break anything?

looks good

we renamed publicly-visible items.

indeed, I know that the channel_mask parameter for evolve will break some of my scripts ...

@cschwan
Copy link
Contributor

cschwan commented Aug 8, 2024

We need a strategy for merging this, since #302 removes all Python files and thus the changed documentation. So either I

  1. merge this PR first, then Use PyO3 functionality to remove Python files #302, which means that in Use PyO3 functionality to remove Python files #302 the updated documentation has to be put in front of the Rust structs (probably),
  2. or I merge Use PyO3 functionality to remove Python files #302 first, which means the changes from this PR will have to be rewritten here.

@felixhekhorn @Radonirinaunimi what path do you believe is best?

@felixhekhorn
Copy link
Contributor Author

  1. merge this PR first, then

I'm in favour of option 1:

if we can resolve #302 quickly they can go into one version (as both are breaking)

@Radonirinaunimi
Copy link
Member

If this is ready it could be merged first and then I will resolve the conflicts in #302. The advantage of this is that this will address some of the todos in #302. If instead #302 is ready first then we reverse the order.

@cschwan cschwan merged commit 4b67c34 into master Aug 9, 2024
9 checks passed
@cschwan cschwan deleted the update-py-docs branch August 9, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated/incomplete Python docs
3 participants