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

Allows 'append' on non-existing plot for option re-use #391

Closed
wants to merge 5 commits into from

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Jun 19, 2018

Closes #100.

People want a blankboard so that they can use the same command in every call of an evaluation loop to plot rather than needing to instantiate the graph before they can update. Existing workarounds are doing a regular plot call on the first iteration and appends for all others, or initializing the plot with a garbage value (like 0,0 on a line) and then appending from that point. Having an option that can create or append solves the issue.

This leverages win_exists to determine whether a particular call should be a create or append when passed the 'createappend' option. It then does the correct action. Tested for both line and scatter, and existing behavior is maintained.

@lvdmaaten
Copy link
Contributor

Do we really need this? Afaict it is currently perfectly well possible to write only a single call for both plot creation and updating?

# this is run this only once:
winid = None  

# ....

for _ in range(10):
    # this is run each time I'm creating or updating my plot:
    winid = vis.line(..., opt={...}, win=winid)

I'm not against adding the feature, but am generally a bit wary of bloat when things like this are being added.

@malmaud
Copy link
Contributor

malmaud commented Jun 23, 2018

You can't currently pass in win=None and update='append', which is where this is useful:

From __init__.py:913:

        elif update is not None:
            assert win is not None

@lvdmaaten
Copy link
Contributor

Right. So perhaps the "right" change to make here would be to allow folks to append to empty plots rather than adding a separate createappend option? This would basically mean that the createappend in the PR would replace the implementation of the current append option.

@malmaud
Copy link
Contributor

malmaud commented Jun 23, 2018

Sounds fine by me.

I suppose database APIs usually make a strong distinction between updating and upserting so users are immediately alerted to logic errors in their code if they try to update a non-existing record, but in the context of visdom the increase in API complexity of the distinction might not be warranted.

@JackUrb
Copy link
Contributor Author

JackUrb commented Jun 25, 2018

Will make the change to append creating by default

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@JackUrb is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@JackUrb JackUrb deleted the upsert-scatter branch August 16, 2018 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants