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

Add R Snippet #6

Merged
merged 7 commits into from
Jan 29, 2021
Merged

Add R Snippet #6

merged 7 commits into from
Jan 29, 2021

Conversation

xhluca
Copy link

@xhluca xhluca commented Jan 27, 2021

Hi @rpkyle , I added this R snippet (the same python snippet is right above). If you have time could you take a look?

@xhluca
Copy link
Author

xhluca commented Jan 27, 2021

This is the python snippet:

import dash
import dash_vtk
import dash_html_components as html

app = dash.Dash(__name__)

app.layout = html.Div(
    style={"width": "100%", "height": "calc(100vh - 16px)"},
    children=dash_vtk.View([
        dash_vtk.GeometryRepresentation([
            dash_vtk.Algorithm(
                vtkClass="vtkConeSource",
                state={"resolution": 64, "capping": False},
            )
        ]),
    ]),
)

if __name__ == "__main__":
    app.run_server(debug=True)

Copy link
Collaborator

@jourdain jourdain left a comment

Choose a reason for hiding this comment

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

I don't know R but that looks good so far.

Copy link
Contributor

@rpkyle rpkyle left a comment

Choose a reason for hiding this comment

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

I think we should revisit the naming of dashVtk::View before merging this in, since it's likely to cause headaches as-is. Otherwise only minor edits to the README.md.

For additional context:

https://www.rdocumentation.org/packages/utils/versions/3.6.2/topics/View

Comment on lines +58 to +60
library(dash)
library(dashVtk)
library(dashHtmlComponents)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally 👌 for the moment, but once the unification PR is merged (plotly/dashR#243), you'll just have to omit the third line, so instead users will load

library(dash)
library(dashVtk)

README.md Outdated

layout <- htmlDiv(
style = list("width" = "100%", "height" = "calc(100vh - 16px)"),
children = View(list(
Copy link
Contributor

@rpkyle rpkyle Jan 27, 2021

Choose a reason for hiding this comment

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

In R, View is a function exported from the namespace of the utils package, so this won't work 🙃 I think you might have meant instead

Suggested change
children = View(list(
children = dashVtk::View(list(

However, I would strongly discourage overloading View, as this function is essentially loaded in every new R session.

My suspicion is that CRAN may reject the package with the function named as it is, because of namespace collisions. (I would, if I were them! This is not obvious unless you're an R user, so no worries for not catching this.)

Is it possible to rename View?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes to rename View but to what?

VtkView?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the capitalization isn't essential for the package, making view (and other functions) lower case would sidestep this issue since object names are case-sensitive in R.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hum, I thought we had to be Capitalize (by convention) on the react side. But maybe we can lowercase them? @xhlulu or else what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jourdain Would it be easier to make all the function names lower case instead? It's a bit more idiomatic to use lower case names for functions in R anyhow, it's not required but an informal convention reflected in current style recommendations:

http://adv-r.had.co.nz/Style.html
https://journal.r-project.org/archive/2012-2/RJournal_2012-2_Baaaath.pdf

Exceptions include classes (like the Dash R6 class 🙂) and a few core functions like Map, Reduce, View, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way (config/setting) to have those component lowercase in R and Capitalized in Python?

Copy link
Contributor

@rpkyle rpkyle Jan 27, 2021

Choose a reason for hiding this comment

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

@jourdain we could also invoke the prefix option for dash-generate-components, so this would instead be generated automatically as

vtkView

which probably resembles most other Dash for R component libraries (besides, say, dashCanvas, which uses no prefix). No changes on the Python side, thankfully.

To do that, we'd just need to specify --r-prefix=vtk when running dash-generate-components, with no extra work beyond that from you or @xhlulu. 🎉

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you already have vtk as a prefix for both R and Julia, so I'm surprised that this would work as shown here... Also I notice that both in R (the R and man directories) and in Julia (src/*.jl) there are multiple versions of the same component build artifacts committed. We should really change the boilerplate so that it wipes these before building new ones in case either the prefix changes, a component name changes, or a component is deleted... but since we don't do that, can you clear these out manually please, and regenerate?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't tested the code but I can try to install R and run it.

Copy link
Author

Choose a reason for hiding this comment

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

@alexcjohnson @rpkyle I will make a PR to wipe & regenerate the package (seems like the prefix is already fine by default, so I just need to correct the snippet). @jourdain is still working on the source code so I'll wait a bit in order to not disrupt his workflow

@xhluca
Copy link
Author

xhluca commented Jan 28, 2021

@rpkyle @alexcjohnson I removed all the old py/R/Rd/jl files, and the NAMESPACE was also regenerated (see PR #11) This is the new namespace; would the new syntax be vtkView or dashVtk::vtkView?

Co-authored-by: Ryan Patrick Kyle <[email protected]>
@rpkyle
Copy link
Contributor

rpkyle commented Jan 28, 2021

@rpkyle @alexcjohnson I removed all the old py/R/Rd/jl files, and the NAMESPACE was also regenerated (see PR #11) This is the new namespace; would the new syntax be vtkView or dashVtk::vtkView?

🏅 Either works, but the latter is often used when a package is not attached to the current session, or to avoid namespace collisions (which we had before with utils::View and dashVtk::View). We can just leave references to vtkView as is, assuming we've already invoked library(dashVtk) earlier in the code.

@xhluca
Copy link
Author

xhluca commented Jan 28, 2021

@rpkyle Thanks, I pushed the new demo:

library(dash)
library(dashVtk)
library(dashHtmlComponents)

layout <- htmlDiv(
    style = list("width" = "100%", "height" = "calc(100vh - 16px)"),
    children = vtkView(list(
        vtkGeometryRepresentation(
            vtkAlgorithm(
                vtkClass = "vtkConeSource",
                state = list("resolution" = 64, "capping" = FALSE),
            )
        )
    ))
)

app <- Dash$new()

app$layout(layout)

app$run_server()

@rpkyle
Copy link
Contributor

rpkyle commented Jan 29, 2021

@rpkyle Thanks, I pushed the new demo:

@xhlulu Cool, it works for me as well! I'd recommend rewriting this as

app <- Dash$new()

app$layout(htmlDiv(
    style = list("width" = "100%", "height" = "calc(100vh - 16px)"),
    children = vtkView(list(
        vtkGeometryRepresentation(
            vtkAlgorithm(
                vtkClass = "vtkConeSource",
                state = list("resolution" = 64, "capping" = FALSE),
            )
        )
    ))
))

app$run_server()

@rpkyle
Copy link
Contributor

rpkyle commented Jan 29, 2021

Looks good to me! 💃

@rpkyle rpkyle self-requested a review January 29, 2021 02:05
@xhluca xhluca merged commit 4d434bf into master Jan 29, 2021
@xhluca xhluca deleted the add-r-example branch January 29, 2021 16:59
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.

4 participants