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

Publish breed agent set changes for use by the NW extension #400

Merged
merged 7 commits into from Aug 5, 2013

Conversation

nicolaspayette
Copy link
Member

This pull request addresses #399. It was originally just a commit on the 5.0.x branch (523b890) but I reverted it (0fe90a8) and turned it into a pull request in order to address the following questions by @SethTisue with further commits and comments (to come):

  • what about World3D? (changes to World often require corresponding changes to World3D)
  • where is the test coverage?
  • this code runs every time a turtle or link is born or dies, right? is there any performance impact on our benchmark suite?
  • (what does scala.collection.mutable.Publisher look like? I don't know anything about that class in particular, there's some pretty janky code in the Scala stdlib, so I hesitate and/or investigate whenever adding a dependency on some new part of it)

@TheBizzle
Copy link
Member

So... am I supposed to merge this right now, or is it waiting on more things to come?

@nicolaspayette
Copy link
Member Author

Waiting for more.

@nicolaspayette
Copy link
Member Author

  • what about World3D? (changes to World often require corresponding changes to World3D)

d7ae29e

  • where is the test coverage?

cea8012
9f35a47

  • (what does scala.collection.mutable.Publisher look like? I don't know anything about that class in particular, there's some pretty janky code in the Scala stdlib, so I hesitate and/or investigate whenever adding a dependency on some new part of it)

https://github.com/scala/scala/blob/v2.9.2/src/library/scala/collection/mutable/Publisher.scala
https://github.com/scala/scala/blob/v2.9.2/src/library/scala/collection/mutable/Subscriber.scala

I'm not sure it's the most well-maintained part of the Scala stdlib, but it seems like fairly straightforward code to me. Also, I've already used them for the plotting and drawing layer event sourcing in master. If there was a problem with them (I don't think there is) we'd need to address it there as well.

  • this code runs every time a turtle or link is born or dies, right? is there any performance impact on our benchmark suite?

Here are benchmarks from my laptop from before and after the change:

                before  after   diff     pct
      Ising     3.648   3.328   -0.32    -8.77%
   Flocking     2.568   2.508   -0.06    -2.34%
       Team     2.525   2.49    -0.035   -1.39%
Bureaucrats     2.415   2.385   -0.03    -1.24%
   GridWalk     5.207   5.153   -0.054   -1.04%
         BZ     5.342   5.302   -0.04    -0.75%
       Fire     0.175   0.174   -0.001   -0.57%
    FireBig     4.074   4.065   -0.009   -0.22%
  GasLabNew     4.559   4.559    0        0.00%
 PrefAttach     4.584   4.587    0.003    0.07%
       CA1D     4.548   4.551    0.003    0.07%
  GasLabOld     3.201   3.216    0.015    0.47%
   VirusNet     1.266   1.272    0.006    0.47%
   Termites     3.119   3.135    0.016    0.51%
 GasLabCirc     4.68    4.709    0.029    0.62%
     Wealth     3.688   3.711    0.023    0.62%
    Erosion     4.053   4.113    0.06     1.48%
ImportWorld     0.985   1.006    0.021    2.13%
       Life     5.137   5.339    0.202    3.93%
   Heatbugs     2.711   2.85     0.139    5.13%
       Wolf     3.747   4.022    0.275    7.34%
       Ants     4.358   4.863    0.505   11.59%

I am, however, skeptical of those results: two of the most impacted models, Heatbugs and Ants, have no agent births or deaths except in setup, and it doesn't seem like this could account for the difference. Those benchmarks, however, where obtained by only waiting for the first "good" result. I think I'm going to heed Seth's advice regarding #403 (comment), bring back the infinite loop, and let the benchmarks run overnight. Doing this for both the "before" and the "after" brings us to Saturday. Stay tuned...

@SethTisue
Copy link
Collaborator

you've addressed my other concerns, so just waiting on the new performance numbers now.

@nicolaspayette
Copy link
Member Author

Here are new numbers, after running both branches over night:

             old    new    diff     pct
      Ising  3.74   3.609  -0.131  -3.50%
     Wealth  3.641  3.593  -0.048  -1.32%
   Flocking  2.493  2.478  -0.015  -0.60%
   Termites  3.078  3.06   -0.018  -0.58%
       CA1D  4.496  4.484  -0.012  -0.27%
   Heatbugs  2.771  2.77   -0.001  -0.04%
    FireBig  4.058  4.06    0.002   0.05%
   GridWalk  5.142  5.146   0.004   0.08%
ImportWorld  1      1.002   0.002   0.20%
       Wolf  3.651  3.659   0.008   0.22%
 GasLabCirc  4.604  4.616   0.012   0.26%
 PrefAttach  4.517  4.53    0.013   0.29%
  GasLabNew  4.47   4.485   0.015   0.34%
Bureaucrats  2.369  2.379   0.01    0.42%
       Fire  0.173  0.174   0.001   0.58%
       Team  2.474  2.49    0.016   0.65%
   VirusNet  1.248  1.259   0.011   0.88%
       Ants  4.243  4.284   0.041   0.97%
    Erosion  3.963  4.002   0.039   0.98%
  GasLabOld  3.155  3.198   0.043   1.36%
         BZ  5.227  5.309   0.082   1.57%
       Life  5.024  5.221   0.197   3.92%

Life is the only one that still seems negatively affected, but since it's a model that has no turtles/links births/deaths whatsoever, I would assume that we can chalk it up to a benchmarking artifact.

If no one has further concerns, I am requesting merging.

@SethTisue
Copy link
Collaborator

LGTM

import scala.collection.mutable.Publisher

sealed abstract trait SimpleChangeEvent
case object SimpleChangeEvent extends SimpleChangeEvent
Copy link
Member

Choose a reason for hiding this comment

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

Why mark the trait as abstract? And why does the trait exist at all? If you want to parameterize Publisher over the type of your case object, you can just say Publisher[SimpleChangeEvent.type].

I had two very similar similar SimpleChangeEvent subscriber classes for
testing in TreeAgentSetTests and AbstractTestWorld. Now it's just one
class used in both places.
@nicolaspayette
Copy link
Member Author

@TheBizzle: It seems like your comment on SimpleChangeEventPublisher went away when I pushed a new commit (?) (OK, I see it now...)

Anyway, here's my answers:

Why mark the trait as abstract?

An abstract trait is, of course, redundant. I probably started with an abstract class and forgot to remove abstract when later changing to a trait. (Or maybe I was momentarily confused... it has been known to happen to me...)

And why does the trait exist at all? If you want to parameterize Publisher over the type of your case object, you can just say Publisher[SimpleChangeEvent.type].

Well, seems like I just learned a new thing about Scala! Thanks! 1ad6dc9

TheBizzle added a commit that referenced this pull request Aug 5, 2013
Publish breed agent set changes for use by the NW extension
@TheBizzle TheBizzle merged commit 92974e6 into 5.0.x Aug 5, 2013
@nicolaspayette nicolaspayette deleted the issue-399 branch August 5, 2013 22:43
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.

None yet

3 participants