Open
Conversation
jwaiton
requested changes
Mar 13, 2026
Member
jwaiton
left a comment
jwaiton
left a comment
There was a problem hiding this comment.
First read over, its good work and very well commented. I just have a few questions.
I couldn't figure out why we need absolute imports here
Running setup.py directly is deprecated:
********************************************************************************
Please avoid running ``setup.py`` and ``easy_install``.
Instead, use pypa/build, pypa/installer or other
standards-based tools.
See pypa/setuptools#917 for details.
********************************************************************************
I guess hypothesis' heuristics, tend to generate values around 0, which makes it filter a lot of data
The new pandas version issues a warning because `fillna` attempts to work on columns of type `str` (a.k.a `object`) which doesn't work. The fix consists in filling `nan`s only in the columsn that might have them.
Hypothesis found the case where x_min==x_max, which we want to test separately.
The data in the input file contains hits that do not correspond to sensor positions. This is not expected in our code. The refactored version of this function assumes that each hit correspond to a SiPM.
This is a mistery. What's changed in pandas(?) hdf5(?) for the data to be in a different order? I have no clue.
The former is deprecated.
We do not avoid inplace overall, but rather column operations with the inplace argument which is no longer allowed. In the new pandas version, the interface forces you to do one of: - do the operation on the dataframe, using the arguments to restrict the operation to a subset of columns (first hunk of the diff) - do the operation on the column, without inplace, and reassign it to the same column (second hunk of the diff)
This has to do with a change in the groupby-apply behaviour of pandas. In the older version, the groups passed to `apply` contained the grouping columns. The new behaviour promotes removing the grouping columns from the groups. For now it still allows both uses via the `include_groups` argument, but it will become the default (or even fixed) behaviour in the future. Before the function used in `apply` (let's say `drop`) was returning a dataframe that contained the grouping columns. `apply` would then combine the resulting dataframes using the grouping columns as index. Finally, we were resetting and dropping the _entire index_ because the grouping columns were already in the dataframe. Now, the grouping columns are not passed to `drop`, so the dataframe it returns does not contain them. When `apply` combines these dataframes using the grouping columns, we must preserve the index in order not to lose this information. However, the dataframe produced in `drop` contains another (meaningless) index. This is just because dataframes must have an index. What we do in this case is to just drop the meaningless index and restore the grouping columns which have been set as the index.
not sure why this is needed, I forgot.
A extreme case was failing the test. We are not interested in that and there was not much we could do, since it's a numpy function that's acting weird.
iloc operations now return wrapped types
This reverts commit b224dfd893cacc38a40088936ed4637319185a51. This doesn't seem to have an effect anywhere, so there is no reason for the explicit conversion
pandas no longer allows indexing using sets
jwaiton
approved these changes
Mar 17, 2026
Member
jwaiton
left a comment
jwaiton
left a comment
There was a problem hiding this comment.
This PR bumps the python version from 3.8 to 3.13, requiring some alterations to the code to ensure IC functions as intended.
Very nice work!
Collaborator
Author
|
Closes #920. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Several changes, particularly in pandas, mean that we need to do things slightly differently.