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

Binning bug #98

Merged
merged 6 commits into from
May 15, 2023
Merged

Binning bug #98

merged 6 commits into from
May 15, 2023

Conversation

zain-sohail
Copy link
Member

Just setting up PR for #95 so we can figure this out. I came with a rough fix that shouldn't be used.

Note: Build will fail since test is testing num_steps.

@rettigl
Copy link
Member

rettigl commented May 8, 2023

I implemented now a version close to what I discussed in #95. I did not include the extension of the number of bins by one, but this can be easily added. What it does not is the following:

  • For ranges, bins, it returns bin centers, aligned at (left range, ... right range except last bin).
    Example: (0, 10), 10 gives [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] as bin centers.
  • For bins as arrays, they are interpreted as bin centers, and converted into edges before binning.
    I also cleaned up the simplify_binning_arguments function, and wrote a test function covering all cases. I removed the case that bins can be a single tuple of bins and ranges, because I don't see a purpose for it (if you have multiple axes, they will most certainly never have the same ranges...).

Copy link
Member

@steinnymir steinnymir left a comment

Choose a reason for hiding this comment

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

Again the same problem... 40 files changed.
This PR is about the bug in the binning function, so why does it refactor almost all docstrings of all functions? are these really changes here or do they come from merging other PRs in here?

The code changes I was able to find looked good. But I am certain I missed some.

For the sake of moving on, I will accept this. But lets try keep bug fix PRs to the point.

comments:

  • I agree with the removal of the tuple method in the binning arguments. I added far too many options, but that is the default from numpy...
  • for other definitions, we can continue our discussion in Binning with Tuple produces unexpected behavior #95 and make smaller, targeted PRs eventually.

@rettigl
Copy link
Member

rettigl commented May 10, 2023

Again the same problem... 40 files changed.

My apologies, this is a mistake in rebasing/merging of upstream I apparently made, which overwrites a lot of recent changes in master. Thanks for spotting this. Rebasing and force pushing should solve this, I believe.

@rettigl rettigl force-pushed the bugfix/binning-tuple branch from 6450199 to 498df55 Compare May 10, 2023 13:52
@rettigl
Copy link
Member

rettigl commented May 10, 2023

Now it is properly rebased, I believe.

Copy link
Member

@steinnymir steinnymir left a comment

Choose a reason for hiding this comment

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

Nice! now I managed to read through easily! 😄
looks good to me!

tests/test_binning.py Outdated Show resolved Hide resolved
@rettigl
Copy link
Member

rettigl commented May 11, 2023

Before merging this, I suggest we finish the discussion of what we want the bins, ranges and tuples functionalities to do, and implement this. At the moment it is np.linspace(r[0], r[1], b, endpoints=False). This produces the same number of points as you give, and with the stepping one might expect (range difference/bins). However, without the last point.

@steinnymir
Copy link
Member

I think this is a good definition. It kind of sounds pythonic, when you think of how range works for example. We could have this as default behaviour, and add the endpoint argument to it allowing to include the last point when really necessary.
I believe there will be very few cases in which this would be used, but almost certainly someone who will complain if it is not there...

@rettigl rettigl force-pushed the bugfix/binning-tuple branch from b66b51c to 69c14ab Compare May 13, 2023 21:46
@rettigl rettigl merged commit 462bbeb into main May 15, 2023
@rettigl rettigl deleted the bugfix/binning-tuple branch May 15, 2023 18:18
@rettigl
Copy link
Member

rettigl commented May 15, 2023

I opened an issue for further improvements, and am mergin this.

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

Successfully merging this pull request may close these issues.

3 participants