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

new option 'spades.debug' #5

Closed
achubaty opened this Issue Nov 1, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@achubaty
Copy link
Contributor

achubaty commented Nov 1, 2017

After our discussion today, I realized we could add a package option spades.debug to allow a user/developer to use their preferred debug setting in when calling spades() with debug.

For developers who prefer debug = TRUE to be default, they can set the option once in their session rather than having to specify debug manually each time.

Would this be a useful addition?

@eliotmcintire

This comment has been minimized.

Copy link
Contributor

eliotmcintire commented Nov 1, 2017

Yes. Can we set default to debug = TRUE ?

Currently, we are all developers; very few R end users yet.

@achubaty

This comment has been minimized.

Copy link
Contributor Author

achubaty commented Nov 1, 2017

I don't agree that setting the default to TRUE is the correct behaviour, even if it might be what you want while in development. Setting the default to FALSE is best/easiest for the user, and developers just need to change the option when they're in development mode.

@eliotmcintire

This comment has been minimized.

Copy link
Contributor

eliotmcintire commented Nov 1, 2017

Every person I have talked too is confused by the spades function because nothing happens. And there is no easily interpretable output of the spades function for a beginner. So, if somebody runs it, then nothing happens, then they get a mega simList output after some unknown amount of time, means that people just give up.

Thinking about visualizing the Event Queue as a debugging thing is the wrong way to see it. It should be considered as a component of the output/return of the function. So, really, debugging is not just showing the Event Queue, it is showing anything else in addition to the Event Queue, i.e., spades(simList, debug = "print(sim$objectValue)") . The package is called Spatial Discrete Event Simulator. The Event Queue is not a tool for debugging; it is the entire point of SpaDES.

I guess, we can see what others think.

@achubaty

This comment has been minimized.

Copy link
Contributor Author

achubaty commented Nov 1, 2017

Part of my issue with doing so is that printing it out after every event drastically reduces the speed of the simulation, especially in Rstudio. Perhaps we should look at minimizing the output displayed, or relegating the printing of output to a future/promise so it doesn't interfere with the main simulation process.

As far as the event queue being part of the output, it is. The simList object contains the current and completed event queues. Strictly speaking the printing of the queue at each event is a side-effect. We are invoking this side-effect as part of the debugging mechanism, which I don't think should be enabled by default.

@achubaty

This comment has been minimized.

Copy link
Contributor Author

achubaty commented Nov 1, 2017

Additionally, if users are getting confused that the spades() call doesn't spit anything out right away, perhaps we should look at making a progress bar or similar the default (as long as it doesn't slow things down). Any simulation with plots, for example, will still produce plots, so that should indicate to the user that something is happening...

@eliotmcintire

This comment has been minimized.

Copy link
Contributor

eliotmcintire commented Nov 1, 2017

If you are worried about speed, then you can set the debug = FALSE. Speed is relatively low on the list for first time users of SpaDES. Power users can do whatever they want. The default is not for power users. If is for average users.

I think using the futures package is good in principle, but currently, I believe it would be even slower for this use case than just printing the message. i.e., there is an overhead that is non trivial.

Progress bars are slow (graphical) or less useful that the Event Queue (text progress bar) and about the same speed. So, progress bars are not a great solution.

achubaty added a commit that referenced this issue Nov 2, 2017

add 'spades.debug' option (#5)
@eliotmcintire, I'm keeping `FALSE` as the default for now because changing it to `TRUE` will require additional work to update various tests. I want to come back to this discussion though.
@achubaty

This comment has been minimized.

Copy link
Contributor Author

achubaty commented Nov 2, 2017

per my commit message: I'm keeping FALSE as the default for now because changing it to TRUE will require additional work to update various tests. I want to come back to this discussion though.

@achubaty achubaty added the revisit label Dec 5, 2017

@eliotmcintire

This comment has been minimized.

Copy link
Contributor

eliotmcintire commented Mar 7, 2019

It is now TRUE ... may change to 2, which shows elapsed time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.