search

cytoscape / cyjShiny

An R/shiny widget for cytoscape.js
Other
92 stars 28 forks source link

explore shinytest with scatterplot demo #45

Closed paul-shannon closed 3 years ago

paul-shannon commented 3 years ago

@Dharna5599

  1. create a new shiny app in cyjShiny/explore/shinytestBug/scatterPlotDemo/
  2. it has a button
  3. every time you press the button, a new “random” scatterplot appears
  4. but! initialize the new shiny app with set.seed(17)
  5. then when I run the app, I will see the same scatterplots that you see
  6. this will give us a REPRODUCIBLE test (google this term!)

To start, do not add shinytests, not until I have approved the shiny app (the scatter plot demo with 1 button)

paul-shannon commented 3 years ago

@Dharna5599 Please update me with your progress and problems on this issue, by replying here and @mentioning me. Please write carefully, editing your text as needed.

Dharna5599 commented 3 years ago

Hey @paul-shannon, Please give me some time I am still exploring it , whenever I get stuck ..I will reach you out. Thankyou

Dharna5599 commented 3 years ago

Hey @paul-shannon, I submitted the PR, but it appears that it is displaying two graphs side by side instead of changing with the click of the button. Please assist me in correcting this. I pushed it in cyjShiny/explore/scatterplot.

paul-shannon commented 3 years ago

Will do, Dharna. Thanks for the heads up, and the PR. I will take a close look later today.

On Jun 11, 2021, at 12:04 PM, Dharna Chandrakar @.***> wrote:

Hey @paul-shannon, I submitted the PR, but it appears that it is displaying two graphs side by side instead of changing with the click of the button. Please assist me in correcting this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

paul-shannon commented 3 years ago

see, run and study explore/scatterplot/paulsVersion.R

Some suggestions: shiny can be very confusing. Best thing to do is to start with a tiny and minimal app. Then, adding one line (or very few) at a time, GROW it in the direction you need it to go. When you get the new line (or small group of lines) to work, study everything about the current code, make sure you really truly understand it.

paul-shannon commented 3 years ago

The essential coding strategy:

  1. create the simplest possible program, which eventually can evolve into the program you want
  2. get this simple version working
  3. study it, study it closely. be able to explain the purpose and function of every character in the code(truly)
  4. then, when you have a solid grasp, and only then, add the next tiny feature
  5. go slow, get a solid grasp, don't move ahead until you REALLY UNDERSTAND

That's how you grow a program.

Dharna5599 commented 3 years ago

Sure @paul-shannon, I will go through the way you said. Thanks for the support

Dharna5599 commented 3 years ago

Hey @paul-shannon, I've pushed the PR once more. The random scatter plot does not change this time when we hit the button, and the result is the same as per the requirement. Kindly review it. Directory: cyjShiny/explore/scatterplot/newapp.R

Thanks

paul-shannon commented 3 years ago

Hi Dharna,

Didn’t we agree that you would work directly in the branch, forgo PRs?

On Jun 12, 2021, at 8:12 AM, Dharna Chandrakar @.***> wrote:

Hey @paul-shannon, I've pushed the PR once more. The random scatter plot does not change this time when we hit the button, and the result is the same as per the requirement. Kindly review it. Directory: cyjShiny/explore/scatterplot/newapp.R

Thanks

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Dharna5599 commented 3 years ago

Sorry Paul,

I just wanted to ensure if whether I was on the right track before implementing it directly on branch. I'll work directly on the branch from now onwards. Thank you

paul-shannon commented 3 years ago

Hi Dharna,

Your PR was to the master branch. We don’t want to update the master until your work on shinytests is complete. So how about I decline the PR?

On Jun 12, 2021, at 9:40 AM, Dharna Chandrakar @.***> wrote:

Sorry Paul,

I just wanted to ensure if whether I was on the right track before implementing it directly on branch. I'll work directly on the branch from now onwards. Thank you

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Dharna5599 commented 3 years ago

Yes sure

paul-shannon commented 3 years ago

@Dharna5599, some suggestions.

  1. your newApp.R displays vertical lines, not a scatterplot
  2. the plot does not change when I click the button
  3. you can take better advantage of github by modifying your first app, rather than creating a new file

These 3 items are excellent opportunities for study and learning.
In particular, I suggest you do a close study, and lots of experimentation with

  1. sample. grasp all of its arguments
  2. set.seed: how often do you call it?
  3. modify your original app.R so that it has the contents in newApp.R
  4. then commit those changes to app.R, and git rm newApp.R

I am not convinced that your REALLY CLOSELY studied your version of my suggested code. Or that you tested your code interactively to ensure that it produces a new scatterplot with each button click.

What coding practice guidelines should you adopt? Perhaps simple rules like "before I commit, I review the original requested features, make sure that I have achieved them"

paul-shannon commented 3 years ago

@Dharna5599, I left set.seed out of my little demo, leaving it as an exercise for you to figure out :) Puzzle through this question: how many times in your program do you need to call set.seed? Zero? Once? With every button click?

Dharna5599 commented 3 years ago

Ok @paul-shannon , I will try to figure it out and work on this.

paul-shannon commented 3 years ago

Hi @Dharna5599. set.seed(17) should be called only once, as the shiny app starts up. This gives us reproducability. And also achieves the desired effect: every button press produces a different scatterplot.

The current version is structured like this:

button click;set.seed(17); sample(1:1000, 100)
button.click; set.seed(17); sample(1:1000, 100)
button.click;set.seed(17); sample(1:1000, 100)

This produces the same numbers with every click

This is the structure what we want:

set.seed(17);
button.click; sample(1:1000, 100)
button.click; sample(1:1000, 100)
button.click; sample(1:1000, 100)
Dharna5599 commented 3 years ago

Hey @paul-shannon, You are correct. I was working yesterday and discovered where I was making mistakes and corrected them. In the shiny app, I need to first set the set.seed only once initially in order to get the required result. In the new branch, I've also done the necessary changes.

paul-shannon commented 3 years ago

@Dharna5599. Well done! I just pushed two small changes, nothing of substance: a conditional runApp() at the bottom, and a makefile. This allows us to run the app from bash, simply by typing make.

I have two requests for you now, in continued pursuit of this issue and task, your exploration of shinytest with this now-solid demo. First, a small request to clean up the repo:

And a larger request, getting back to our main line of development:

Let me know if this makes sense, and if you agree that these steps should be your priority now. If you disagree, do not hesitate to push back!

Dharna5599 commented 3 years ago

Yes @paul-shannon, I get a clear understanding of both the goal and the work. In addition, I updated the now-solid demo. Please check to see if I completed everything correctly or if there are any modifications that need to be made.

paul-shannon commented 3 years ago

@Dharna5599 Yesterday we explored how your version of the scatterplot shinytest depended upon RStudio. I explained thatfor automated package building, and for R CMD check - which is needed for submission to both Bioconductor and CRAN - that the shinytests must run from the shell (from bash).

Here is a full working demonstration of that approach, in the abandonWebPack-try2 branch:

cyjShiny/explore/scatterplot/unitTests/shinyTestDemonstration

You will find a detailed README.txt in that directory which explains, step by step, how to create the files and directories required for the automated test, and then how to run the test via make.

Please study this, then create your own test in unitTests/dharnasShinyTestDemonstration/ When you get it working, or if you get stuck, please let me know.

Dharna5599 commented 3 years ago

Sure @paul-shannon Thank you

paul-shannon commented 3 years ago

@Dharna5599 - excellent work figuring out how to do explicit shiny font selection. With this problem solved, and the solution added to dharnashinyTestDemonstration:

This sets you up for the next task, the problem that go us started down this path: figure out why cytoscape.js graphics are not captured by shinytest. I will start a new issue for that, and make some strategy suggestions, by the end of the day here in Seattle, ready for you as you begin your work on Friday.

Good work!

Dharna5599 commented 3 years ago

Thank you @paul-shannon, I have also pushed the required changes you want me to do in dharnashinyTestDemonstration. Kindly review it.

paul-shannon commented 3 years ago

thanks @Dharna5599 . I see your good use of the same font, but alas, the rendering is still slightly different. google shinytest "font" and you will see others have seen the problem. A truly universal solution seems unlikely - or more fuss than we want.

Here is a proposed workaround: a successful test is always run against a recorded test on the same computer

This is a disappointment - to my mind, a deficiency in shinytest.

Let's pause in this strategy, and take a look at another one. I will ask Max Franz about this idea. And I will create a new issue for us to work on - titled "cyjs introspective testing"

paul-shannon commented 3 years ago

this issue taught us that shinytest is a poor match to our needs. "introspective" testing is our new approach, a first example now implemented in unitTests/test_selection.R