DashR Oil and Gas Ternary by KPhans · Pull Request #245 · plotly/dash-sample-apps

KPhans · 2019-07-25T18:46:43Z

Issue for app: #[issue number here]

App pull request

This is a new app
I am improving an existing app (redesigns/code "makeovers")

About

Playground deployment URL (new version):
Current gallery app URL: (delete this line if inapplicable)
Python app repository link: (delete this line if you are working on a Python app)

Workflow

I have created a branch in the appropriate monorepo, and the
elements necessary for successful deployment are in place.
If the app is a redesigned and/or restyled version of an
existing gallery app, I've summarized the changes requested in the
appropriate Streambed issue and confirm that they have been applied.
If the app is on the Dash Gallery portal, I have added a link to
the GitHub repository for the source code in the portal description.
If the app is a reimplementation of a Python gallery app for the
DashR gallery, the app in this PR mimics, as closely as possible,
the style and functionality of the existing app.=
I have removed all Google Analytics code from the app's
assets/ folder.

The pre-review review

I have addressed all of the following questions:

Does everything in my code serve some purpose? (I have removed
any dead and/or irrelevant code.)
Does everything in my code have a clear purpose? (My code is
readable and, where it isn't, it has been commented appropriately.)]
Am I reinventing the wheel? (I have used appropriate packages to
lessen the volume of code that needs to be maintained.)

…rtly, some tweaking needs to be performed.

KPhans · 2019-07-25T18:49:29Z

@rpkyle PR for Ternary app to be merged into master!

rpkyle

The app looks great, and is very responsive, which is fantastic. Awesome work!

Just a few comments to address before moving ahead. One thing that we should sort out is the clipping for the ternary axis label on the left side. Compare the R version:

with the Python version:

There's a bit more space between the figures in the Python version -- it looks like the "Carbonate" label is being clipped -- i.e. the first letter looks slightly chopped off.

Not sure, but this might be a CSS issue?

rpkyle · 2019-07-30T18:23:09Z

+
+```
+git clone https://github.com/plotly/dash-sample-apps.git
+Run app.R in R Studio


We should either make this statement a valid command or remove it from the monospaced code block (to avoid confusion for new users).

rpkyle · 2019-07-30T18:23:38Z

@@ -0,0 +1,50 @@
+## dashr-oil-gas-ternary
+
+`dash-oil-gas-ternary` creates a dashboard for mineral composition evaluations from natural gas wells.  


dashr-oil-gas-ternary

rpkyle · 2019-07-30T18:24:18Z

+git clone https://github.com/plotly/dash-sample-apps.git
+Run app.R in R Studio
+```
+In UNIX system: 


I'd 🔪 lines 19-37, these are Python-related.

rpkyle · 2019-07-30T18:24:49Z

+df = read.csv("data/test_compositionR.csv")
+df_prod = read.csv("data/YearlyProduction_table_1.csv")
+
+colormap = list()


You could replace lines 28-33 with the following:

colormap <- setNames(colors[1:length(unique(df$fm_name))], unique(df$fm_name))

rpkyle · 2019-07-30T18:37:36Z

+      ),
+      showlegend=TRUE
+    )
+    data = list.append(data,new_trace)


We should rename the data object here, because data is an exported function from the namespace of the utils package in R.

Now, as for list.append ... ahem 🙂

Not a criticism of your code at all, but I find this to be a very silly function. Given x <- list(a=1,b=2,c=3):

> list.append(x,d=4,e=5) $a [1] 1 $b [1] 2 $c [1] 3 $d [1] 4 $e [1] 5

compare ☝️ with

> c(x, d=4, e=5) $a [1] 1 $b [1] 2 $c [1] 3 $d [1] 4 $e [1] 5

😄

rpkyle · 2019-07-30T19:19:57Z

+               input("operator-select", "value")),
+             function(map_selected_data, tern_selected_data, op_select){
+               dff = df[df$op %in% op_select,]
+               #browser()


🔪 #browser()

rpkyle · 2019-07-30T19:20:28Z

+               state("ternary-map", "figure")),
+             function(map_selected_data, bar_selected_data, bar_click_data, op_select,
+                      layer_select, curr_fig){
+               #browser()


💣 #browser()

rpkyle · 2019-07-30T19:20:56Z

+                           input("operator-select", "value"),
+                           input("mapbox-view-selector", "value")),
+             function(tern_selected_data, bar_selected_data, bar_click_data, op_select, mapbox_view){
+               #browser()


#browser() 🔫

rpkyle · 2019-07-30T19:21:49Z

+                           input("form-by-bar", "selectedData"),
+                           input("operator-select", "value")),
+             function(map_select, tern_select, bar_select, op_select){
+               #browser()


💥 ☠️ ⚛️ #browser()

rpkyle · 2019-07-30T19:22:03Z

+                 if(length(tern_select) > 0){
+                   processed_data = list('well_id' = list(), 'formation' = list())
+                   for (point in tern_select[['points']]) {
+                     #if('customdata' %in% point){


🔪 commented out line

KPhans · 2019-07-31T14:55:33Z

@rpkyle i believe everything is corrected. As per the css issue, i did not notice it when i ran it on my system.

rpkyle

Looks great to me, just had another look at this app, I believe it's ready to go. Awesome work, definitely nice to see callback_context in action here.

Just need to resolve merge conflicts and then I think you're OK to 💃

HammadTheOne · 2019-08-09T00:33:08Z

+df_fm <- df_fm[-c(1),] 
+
+formation_name <- list()
+i <- 1


I found this interesting, looping using an index counter, I see that a lot in Python but not in R. It works fine, and I think it's more of a stylistic choice, personally I would iteratively loop through the range of elements in R, but I think it's your call how you do it. 👍

for now, will stick with an index counter as it seems to work. definitely will use range or seq later on

HammadTheOne

Hi Kevin, the app code looks pretty solid, and it runs great as Ryan mentioned. The only issue I found was that if I select the "satellite-street" view on the 'Well Map', it seems to break Mapbox and sends a few errors in the browser console and I can no longer return to the other views. Aside from that error, seems like 💃 to me.

KPhans · 2019-08-09T15:21:22Z

@HammadTheOne hmm that is very weird indeed. Ill look into that later today. If all the other options work properly and this one doesn't, then it may not be a problem with the code

KPhans · 2019-08-13T03:00:29Z

@rpkyle @HammadTheOne Fixed the satellite street view. Was a silly filtering where the : was changed to a = in "mapbox://styles/mapbox/satellite-streets-v9". Since R doesn't utilize colons for equate values. Conflicts have been resolved.

rpkyle

💃 LGTM!

KPhans and others added 10 commits June 20, 2019 16:07

initial commit for dashR salesforce CRM. Styles update to be done sho…

285d955

…rtly, some tweaking needs to be performed.

added READ.ME

7c1d45c

initial commit for Oil & Gas Ternary(incomplete.)

10ca72d

Update README.md

477fda9

various app fixes

06fc1c5

source code modifications

65ad224

modified code

53838a6

changed init file

350ccae

Update README.md

0db78d6

Update app.R

50279c0

KPhans requested a review from rpkyle July 25, 2019 18:49

rpkyle changed the title ~~Dashr oil and gas ternary~~ DashR Oil and Gas Ternary Jul 30, 2019

rpkyle added the dash-r Dash R applications label Jul 30, 2019

rpkyle assigned KPhans Jul 30, 2019

rpkyle suggested changes Jul 30, 2019

View reviewed changes

KPhans added 2 commits July 31, 2019 10:52

Update app.R

36a4ed0

Update README.md

925e842

rpkyle requested review from HammadTheOne and rpkyle August 8, 2019 20:08

rpkyle previously approved these changes Aug 8, 2019

View reviewed changes

HammadTheOne reviewed Aug 9, 2019

View reviewed changes

HammadTheOne previously approved these changes Aug 9, 2019

View reviewed changes

Update app.R

5c5a026

KPhans dismissed stale reviews from HammadTheOne and rpkyle via 5c5a026 August 13, 2019 02:41

Merge branch 'master' into dashr-oil-and-gas-ternary

c2bf72f

Merge branch 'master' into dashr-oil-and-gas-ternary

6236203

rpkyle self-requested a review August 22, 2019 13:10

rpkyle approved these changes Aug 22, 2019

View reviewed changes

rpkyle merged commit cf67f4e into master Aug 22, 2019

rpkyle deleted the dashr-oil-and-gas-ternary branch August 22, 2019 13:11

rpkyle restored the dashr-oil-and-gas-ternary branch August 22, 2019 13:17

rpkyle deleted the dashr-oil-and-gas-ternary branch August 22, 2019 13:17

		@@ -0,0 +1,50 @@
		## dashr-oil-gas-ternary

		`dash-oil-gas-ternary` creates a dashboard for mineral composition evaluations from natural gas wells.

Uh oh!

Conversation

KPhans commented Jul 25, 2019

App pull request

About

Workflow

The pre-review review

Uh oh!

KPhans commented Jul 25, 2019

Uh oh!

rpkyle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpkyle Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KPhans commented Jul 31, 2019

Uh oh!

rpkyle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HammadTheOne left a comment

Choose a reason for hiding this comment

Uh oh!

KPhans commented Aug 9, 2019

Uh oh!

KPhans commented Aug 13, 2019

Uh oh!

rpkyle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rpkyle Jul 30, 2019 •

edited

Loading

rpkyle left a comment •

edited

Loading