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

Progress bar fixes #1340

Merged
merged 1 commit into from
Aug 12, 2017
Merged

Progress bar fixes #1340

merged 1 commit into from
Aug 12, 2017

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Aug 8, 2017

Motivation and context:
As noted in #1314 it can take a long time for the build progress bar to appear. This PR partially addresses this problem by fixing some problems:

  • The counting of steps was off so that the progress bar thought the build was completed way to early causing an ETA of 0s (the appearance of the progress bar is based on this estimate exceeding a threshold).
  • The progress bar should not just appear when the ETA exceeds a threshold, but also when enough time has elapsed (e.g. a long time could have elapsed but the ETA alone might be below the threshold because very few steps remain).
  • A significant amount of time can be spend on shrinking the cache and thus I've included this as an additional step in the progress bar (more precisely the whole building of the top-level network is included as a step).

I think this improves the build progress bar enough to merge these changes. However, some issues are left open:

  • The progress bar can still take some time to appear if the first decoder solver takes a long time. I don't think that this can be fixed without spawning a separate thread which will add its own slew of problems. We could also ditch the AutoProgressbar and immediately display the progress bars.
  • It is hard to know how much time will be spent on shrinking the cache which make the ETA inaccurate (especially for small networks, for large networks this should be less of a problem). This could potentially be solved by storing how long the cache shrink took in earlier builds and using that as an estimate. However, this is not super straight forward to implement and I am doubtful that it is worth the developer time and added code complexity.
  • The optimizer run is not included in the progress bar at all (it will run after the build progress bar displays finished). Again, there are some complications with including this into the progress bar at themoment (but maybe less so as for the previous point). Still, I have no idea how to predict the run time or even the number of iterations beforehand ...

Interactions with other PRs:
none

How has this been tested?
I used this script to manually test it on the console:

import nengo

with nengo.Network() as model:
    e1 = nengo.Ensemble(4000, 20)
    e2 = nengo.Ensemble(4000, 20)
    n = nengo.Node(size_in=20)
    nengo.Connection(e1, n)
    nengo.Connection(e2, n)

with nengo.Simulator(model) as sim:
    pass

An automated test seems extremely hard to implement.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • [n/a] I have updated the documentation accordingly.
  • I have included a changelog entry.
  • [n/a] I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

Definitely an improvement. I would probably be in favor of dropping AutoProgressbar, but not 100% sure, and is probably a big enough change to warrant a separate PR.

Also, thanks for including the test script, made testing this PR much easier :)

The callback was counting too many steps because it also gets called
for objects like Synapses that are not in the total object count of
the network.

Additionally, we consider elapsed time in AutoProgressBar and
include the top level net as a step in build progress.
@tbekolay tbekolay merged commit 1304051 into master Aug 12, 2017
@tbekolay tbekolay deleted the progressbar-fixes branch August 12, 2017 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants