Skip to content

Conversation

henryiii
Copy link
Member

Closes #579.

This was an intentional design decision, based on the fact that h + 1 shouldn't add 1 to the flow bins, but scaling makes more sense to apply to all.

@github-actions github-actions bot added the needs changelog Might need a changelog entry label May 20, 2021
@henryiii
Copy link
Member Author

@HDembinski

Currently, we have (idea 1):

h + 1 # only adds to the normal bins
h * 1 # only scales the normal bins

With this PR (idea 2):

h + 1 # only adds to the normal bins
h * 1 # scales all bins (flow too)

Another idea (idea 3):

h + 1 # adds to all bins (flow too)
h * 1 # scales all bins (flow too)

The original idea was based on the fact that we always treat flow as hidden in Python, so adding +1 would be very surprising if it added 1 to the flow bins (which is poorly defined, since they are infinite - but one could argue that adding a constant to variable bins, etc is pretty useless too). But one could argue quite well that scaling should always include the flow bins, which leads into idea 2 (implemented here) or idea 3 being clearly less surprising and better. Note for above examples, / acts like * and inplace versions are included. I'm leaning toward 2, but 3 would be more consistent.

Of course, you can do any of these explicitly using an array, then you get to pick based on the size of the array if the flow bins are included. This is just for the shortcut.

@henryiii henryiii changed the title fix: support scaling flow bins fix//feat: support scaling flow bins May 20, 2021
@henryiii henryiii marked this pull request as draft May 20, 2021 20:53
@henryiii henryiii changed the title fix//feat: support scaling flow bins fix/feat: support scaling flow bins May 28, 2021
@andrzejnovak
Copy link
Member

@HDembinski Can you give feedback? It would be good to resolve this sooner or later. I am in favour of 3) for consistency reasons.

@HDembinski
Copy link
Member

HDembinski commented Jun 11, 2021

Flow bins should not be treated different. If you add 1 it should also include the flow bins. Same with multiplication. The moment you add a constant to a histogram, it stops being a histogram anyway. The least surprising is to treat all bins the same. I was not aware that adding used to work differently and I would have been strongly against.

@HDembinski
Copy link
Member

@HDembinski Can you give feedback? It would be good to resolve this sooner or later. I am in favour of 3) for consistency reasons.

Seems we agree then, thanks for pinging me.

@henryiii henryiii force-pushed the henryiii/fix/scale branch from 5f22937 to a4847d3 Compare June 22, 2021 22:37
@henryiii henryiii force-pushed the henryiii/fix/scale branch from 5e6d8b3 to b939bc6 Compare June 22, 2021 22:40
@henryiii henryiii marked this pull request as ready for review June 23, 2021 00:14
@henryiii henryiii merged commit deffd9f into develop Jun 23, 2021
@henryiii henryiii deleted the henryiii/fix/scale branch June 23, 2021 02:39
@henryiii henryiii removed the needs changelog Might need a changelog entry label Sep 13, 2021
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.

[BUG] Scaling histogram doesn't affect flow bins
3 participants