Skip to content

Conversation

gshotwell
Copy link
Contributor

This creates a slightly more real-world brushing example where the user can visually annotate a plot, and then download the annotated data.

@CLAassistant
Copy link

CLAassistant commented Jun 26, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

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

I like the idea of the app but it seems to not be working for me:

annotation.mp4

Comment on lines 40 to 43
path = Path(__file__).parent / "boulder_temp.csv"
dataframe = pd.read_csv(path)
dataframe["date"] = pd.to_datetime(dataframe["date"])
dataframe["annotation"] = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the CSV reading should happen outside of the server function, at the top level of the app, so that it's read only once. Otherwise it'll be read every time any user changes their copy of annotation_values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call

Comment on lines 91 to 92
selected = selected_data().copy()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this copy() isn't needed.

Now that I think about it, how about updating this app to use copy on write? That should eliminate the need for using .copy().
#614
https://pandas.pydata.org/docs/dev/user_guide/copy_on_write.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a few things, and it turns out that moving some date time processing up in the script means that all of the copies can be removed without needing the pandas copy-on-modify.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like there's still a place where the reactive.Value ts_data() is being modified in place; the modified value is downloaded in download, where it calls ts_data.to_csv(). This results in the expected overall behavior for the app, but it relies on a dangerous modification-in-place. I'll make a fix.

@gshotwell
Copy link
Contributor Author

@wch I think the app was working but maybe poorly documented. The idea was to show a table of annotated values, so if you don't add any annotations the table should be empty. I added a placeholder and heading to make that a bit clearer.

@wch
Copy link
Collaborator

wch commented Jul 17, 2023

I just pushed some small formatting fixes. Note that the br() was added to make things look better for users that aren't using wide browser windows.

@wch wch merged commit f2e0535 into main Jul 17, 2023
@wch wch deleted the plot_annotation_example branch July 17, 2023 17:31
schloerke added a commit that referenced this pull request Jul 18, 2023
* main:
  Rename shiny/examples to shiny/api-examples (and X/examples to X/api-examples) (#627)
  Make --app-path work with app file argument (#598)
  Don't eval example code block
  Fix example code blocks (#626)
  Annotation export example (#584)
schloerke added a commit that referenced this pull request Jul 24, 2023
* main:
  Add E2E tests for accordion and autoresize (#601)
  Add card test (#622)
  Add sidebar test (#631)
  Make card fullscreen icon a tooltip (#632)
  Pull in changes from rstudio/bslib#697 and rstudio/bslib#699
  Changelog tweak. Followup to #629
  Add experimental tooltip methods and example apps (#629)
  Use `blib::bs_theme(5,"shiny")` for py-shiny theme (#624)
  Rename shiny/examples to shiny/api-examples (and X/examples to X/api-examples) (#627)
  Make --app-path work with app file argument (#598)
  Don't eval example code block
  Fix example code blocks (#626)
  Annotation export example (#584)
  Add todo list example (#603)
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.

3 participants