Skip to content

Conversation

@rht
Copy link
Contributor

@rht rht commented Nov 18, 2017

cc: @simonchx, @clmerlos, @JamesArruda, @njvrzm

This fixes the distance calculation back to before this commit change: 600c62b#diff-84ac902399d236cac5d91bacaad69b2bL547, additionally, uses numpy for the arithmetic as suggested by @JamesArruda's implementation. This version of distance calculation passes the most number of test cases.

This fixes #410.

@coveralls
Copy link

coveralls commented Nov 18, 2017

Coverage Status

Coverage increased (+0.05%) to 82.177% when pulling d85599b on rht:torus_distance into 275674d on projectmesa:master.

@simonchx
Copy link

@rht Thanks for the fix! It works well.

@Corvince
Copy link
Contributor

Corvince commented Dec 6, 2017

Thanks @rht. But what do you mean by

This version of distance calculation passes the most number of test cases.

Are there any test cases where the calculation fails?

@rht
Copy link
Contributor Author

rht commented Dec 6, 2017

Are there any test cases where the calculation fails?

There are not. In retrospect, I should have added this point in my description though.

@TaylorMutch
Copy link
Contributor

For clarification (maybe I don't understand), how does this PR differ from #425 ?

@rht
Copy link
Contributor Author

rht commented Dec 7, 2017

#425 fails the test at [1]

pos_6 = (-30, -29)
pos_7 = (21, -5)
assert self.space.get_distance(pos_6, pos_7) ==  np.sqrt(49 ** 2 + 24 ** 2)

[1] #410 (comment)

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage increased (+0.04%) to 82.955% when pulling e50af5c on rht:torus_distance into 67f14ab on projectmesa:master.

@rht rht force-pushed the torus_distance branch 2 times, most recently from 5f07737 to f047ae4 Compare December 7, 2017 03:38
@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage increased (+0.04%) to 82.955% when pulling f047ae4 on rht:torus_distance into 67f14ab on projectmesa:master.

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage increased (+0.04%) to 82.955% when pulling a3014a1 on rht:torus_distance into 67f14ab on projectmesa:master.

@rht
Copy link
Contributor Author

rht commented Dec 7, 2017

I added that test case assert self.space.get_distance(pos_6, pos_7) == np.sqrt(49 ** 2 + 24 ** 2).

@Corvince
Copy link
Contributor

I think this can finally be merged, thanks everyone for the contributions!

@Corvince Corvince merged commit a6c74cf into projectmesa:master Dec 14, 2017
@rht rht deleted the torus_distance branch December 15, 2017 02:19
@jackiekazil jackiekazil added this to the Hayden milestone Jan 6, 2018
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 Report] Wrong result got from get_distance method for torus continuous space

6 participants