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

Milestone 4 Feedback #99

Open
zwarham opened this issue Mar 28, 2023 · 10 comments
Open

Milestone 4 Feedback #99

zwarham opened this issue Mar 28, 2023 · 10 comments

Comments

@zwarham
Copy link

zwarham commented Mar 28, 2023

  • Submission (4)
    o 4/4 mechanics
  • Improving your app (20)
    o 20/20 mechanics
  • Reproducibility – testing (10)
    o 0/5 mechanics
     Not enough test that cases (required 3)
    o 5/5 accuracy
  • Reproducibility – continuous integration and deployment
    o 5/5 mechanics
    o 0/5 accuracy
     Deploy_app.yaml is not passing
  • Tie it all together and deliver a production ready app (50)
    o 20/20 mechanics
    o 10/20 accuracy
     Some cities do not work (eg. Dubois in WY)
     When selecting a city in one state and then following that with another state, it reacts twice (once for the change in state and once for the change in city) which causes the widgets to flash
    o 10/10 visualization
  • Reflection
    o 6/6 reasoning
@eyrexh
Copy link
Collaborator

eyrexh commented Mar 28, 2023

Hi Zac @zwarham ,

Thank you for your feedback. I got some questions regarding the grading. We have 4 tests to pass, which is more than 3 and shows in the passing information. Also, we follow what Florencia taught and the ci/cd passed that our pages can be updated on shiny every time we push the commits. May I know what the Deploy_app.yaml do in this case? Thank you.
Team 3

Screenshot 2023-03-28 at 09 48 44

Screenshot 2023-03-28 at 09 49 39

@renee-kwon
Copy link
Collaborator

Hi @zwarham, could we also get some clarification for our deductions for "Tie it all together and deliver a production ready app"? (10 mark deduction)

  1. There is no city "Dubois in WY" as in your comment. I checked all cities for WY (and other states) and they are all working correctly.
  2. I understand your comment about the flashing of the widgets when switching states, but the milestone description states: "reactive options should be working and not giving any errors." This is what we have with our reactive elements. Could you explain how the flashing resulted in deductions in accuracy?
    Thank you.

@zwarham
Copy link
Author

zwarham commented Mar 28, 2023

Hi Zac @zwarham ,

Thank you for your feedback. I got some questions regarding the grading. We have 4 tests to pass, which is more than 3 and shows in the passing information. Also, we follow what Florencia taught and the ci/cd passed that our pages can be updated on shiny every time we push the commits. May I know what the Deploy_app.yaml do in this case? Thank you. Team 3

Screenshot 2023-03-28 at 09 48 44 Screenshot 2023-03-28 at 09 49 39

Hi, this is the information we look at for the workflows

image

As for the tests, I only see two in the required document - https://github.com/UBC-MDS/citytemp/blob/main/tests/testthat/test-shinytest2.R

@zwarham
Copy link
Author

zwarham commented Mar 28, 2023

Hi @zwarham, could we also get some clarification for our deductions for "Tie it all together and deliver a production ready app"? (10 mark deduction)

  1. There is no city "Dubois in WY" as in your comment. I checked all cities for WY (and other states) and they are all working correctly.
  2. I understand your comment about the flashing of the widgets when switching states, but the milestone description states: "reactive options should be working and not giving any errors." This is what we have with our reactive elements. Could you explain how the flashing resulted in deductions in accuracy?
    Thank you.

Please see attached picture for Dubois - it shows on the map but not in the dropdown

image

The double flashing is because your widgets react to multiple sources that change each other. Your children elements are reacting on each other instead of their parents. You should either have a hierarchical react order or a delay to avoid reacting more than once

@eyrexh
Copy link
Collaborator

eyrexh commented Mar 28, 2023

Hi @zwarham ,

I think you should not check the commit message for a specific file since when we merge the pull request too soon, the merge also triggers one deployment so the pull request one failed due to multiple deployment. Please check our action workflow at last and also our badge shows ci/cd works.

For the tests, I think she mentions the number should be the “PASS” one. I created both screenshots and value checking for 2 actions so it is 2*2=4 tests. Thank you.

Team 3

@zwarham
Copy link
Author

zwarham commented Mar 28, 2023

Hi @zwarham ,

I think you should not check the commit message for a specific file since when we merge the pull request too soon, the merge also triggers one deployment so the pull request one failed due to multiple deployment. Please check our action workflow at last and also our badge shows ci/cd works.

For the tests, I think she mentions the number should be the “PASS” one. I created both screenshots and value checking for 2 actions so it is 2*2=4 tests. Thank you.

Team 3

Unfortunately these are the instructions provided by Florencia for grading. If you disagree, please create a regrade request on Slack by creating a group conversation with Florencia, myself and your group members and explain to Florencia your reasoning.

@flor14
Copy link

flor14 commented Mar 28, 2023

Yes, sometimes the GitHub action workflow can fail due to multiple deployments, that is correct.
The app is MUCH BETTER than the one you showed me in the last lab. Well done with the improvements. Congratulations!

@eyrexh
Copy link
Collaborator

eyrexh commented Mar 29, 2023

Thank you for your feedback @flor14 . Just to double confirm that you mean the multiple deployments cause that specific commit failed is a one-time issue and our workflow works great for all the later commits, so we can get full marks for the ci/cd. Thank you!

Reproducibility – continuous integration and deployment
o 5/5 mechanics
o 0/5 accuracy
 Deploy_app.yaml is not passing

@snesunil
Copy link
Collaborator

snesunil commented Apr 1, 2023

Hi @flor14,

Thank you for reviewing the above request 😊. We also wanted to discuss the following items -

a) The 10 marks deduction in the "Tie it all together and deliver a production-ready app" - accuracy.

The information shown on the page is accurate based on the selection in the drop-downs. As per the design, the cities drop-down is populated with the cities in the current selected state. We are considering the state drop-down as the parent element here. Hence, if a state is changed, the cities drop-down will get updated and the reactive elements also will be updated as per the selected state and first item in the city dropdown.

b) 5 marks deduction in Reproducibility – testing

We have implemented 2 testthat functions that works as expected. But 0 marks were given for mechanics in this section.
Could you please check if we are eligible for additional points as 15 marks in total were deducted?

This is the link to the app.

Thank you for your time and consideration,
Group 3

@flor14
Copy link

flor14 commented Apr 5, 2023

Regrading request:

a) The 10 marks deduction in the "Tie it all together and deliver a production-ready app" - accuracy.

I will consider this discount correct based on 2 aspects:

  • The UI could be better organized. When you choose different cities the map of the country does not change. It would be better to (a) use tabs or (b) organize the interface in such a way that it is understood that the dropdown menus are affecting only the line chart and the boxes. We talked about this in the laboratory.
  • The "Select Temperature" radio button is available when you are plotting precipitation. You could create a dynamic UI to make this button appear only when you want to plot temperature.

b) deduction in Reproducibility – testing

I have returned 10 points for the workflows because this is a grading error

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

No branches or pull requests

5 participants