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

Fix for Inconsistent Uneven Grid Distributions #167

Merged
merged 5 commits into from
Sep 18, 2018
Merged

Fix for Inconsistent Uneven Grid Distributions #167

merged 5 commits into from
Sep 18, 2018

Conversation

JosephTomlinson
Copy link
Contributor

This closes #166
Was an unexpectedly minor change.
If anyone more familiar with edge cases can test this I would appreciate it, but I think given the behavior of range and the previously inconsistent distribution before this shouldn't break anything.

Properly produces 3,2,2 distribution for 7 split three ways and 2,2,2,1 for 7 split four ways.

@vchuravy
Copy link
Member

Thanks! Would be good to have a couple of tests for this behaviour to make sure it doesn't regress again!

@JosephTomlinson
Copy link
Contributor Author

I've added a simple test. Honestly unsure how to add any more but I think this should suffice.

@vchuravy vchuravy merged commit 9b6111b into JuliaParallel:master Sep 18, 2018
@JosephTomlinson
Copy link
Contributor Author

So as I was using this in some other code and I discovered that it's actually incorrect for an edge case.
Specifically if range outputs a whole number then you still get uneven distributions.
An example being a length 50 array split over 4 gives 13,12,13,12.

I have a solution, it's a bit inelegant as it uses a for loop, but it should be strictly correct since we are no longer relying on rounding based on range output.

This is my first time in this situation so I'm not sure what the best way to push the fix is.

@vchuravy
Copy link
Member

vchuravy commented Sep 18, 2018

You will need to open another PR. So checkout current master locally and branch of there, add your fix + test and open a new PR for that.

Ah I see you worked on master for this PR.
So do something like this:

git fetch
git checkout origin/master
git checkout -b jt/myfix
# Then do your fix
git push -u yourremote jt/myfix

and open a new PR. You can also do git checkout master; git reset --hard origin/master to get back in sync with upstream, but that will lose you changes you might have made.

vchuravy pushed a commit that referenced this pull request Nov 16, 2018
* Fixes Edge Cases with #167
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.

Unintuitive/Inconsistent Distribution for Uneven Grids
2 participants