-
Notifications
You must be signed in to change notification settings - Fork 34
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
Update the new document specifying working with public dashboard. #1056
Update the new document specifying working with public dashboard. #1056
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small suggestion on breaking up a bullet point but other than that looks great to me! I think this will be a great resource for people working on the public dashboard so we can easily find what we need to know for development and testing!
- Make sure the default charts are loaded at the first instance of loading the website. | ||
- Make sure all the charts are being launched or the error table is displayed. | ||
2. Test with loading and unloading different program/study dataset. | ||
- Load different datasets, run the scripts to execute the notebooks to make sure the charts are being generate properly. Ensure to unload the datasets, and re-load the new dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to put the step between loading in it's own bullet, like:
... make sure the charts are being generated properly
- Between datasets, drop the old dataset and load the new dataset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iantei thanks for submitting this - I always like to see new documentation!
However, I am very reluctant to duplicate code in the documentation unless we have somebody signed up to keep the documentation in sync.
Bad/obsolete documentation is just as bad as no documentation.
For concrete examples, see below.
Once you remove the duplicated code, I am happy to merge!
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/update_mappings.py mapping_dictionaries.ipynb | ||
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py generic_metrics.ipynb default | ||
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py generic_metrics_sensed.ipynb default | ||
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py generic_timeseries.ipynb default | ||
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py mode_specific_metrics.ipynb default | ||
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py mode_specific_timeseries.ipynb default | ||
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py energy_calculations.ipynb default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list of notebooks will become obsolete as soon as the stacked bar charts change is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed reference to all notebooks, while just keeping mapping_dictionaries.ipynb
and generic_metrics
. Added a note to reference notebooks from crontab
in em-publicdashboard
.
services: | ||
notebook-server: | ||
environment: | ||
- DB_HOST=mongodb://db/openpath_prod_usaid_laos_ev | ||
- WEB_SERVER_HOST=0.0.0.0 | ||
- CRON_MODE=TRUE | ||
- STUDY_CONFIG=usaid-laos-ev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may become obsolete once the changes discussed in #1048 are checked in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since, there's still progress ongoing for changes related to #1048 , I have included a note to update this once the changes re checked-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1048 is being actively worked on, and will be merged in the next month or so. And the next time it is updated, this will need to be updated as well. I don't understand why this is so important to be replicated here.
As I said before:
Bad/obsolete documentation is just as bad as no documentation.
Why does this need to be replicated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be replicated here?
At present, while documenting the process of working with Public Dashboard, this is a rightful approach of changing the dataset and study config in the docker-compose file. Therefore, I felt it would be rightful to keep this part of documentation (provided reference that change would be need to be made when the #1048 is done). Considering document would be updated when changes are made on the the referred issue.
But accounting for the future prospect of being obsolete, the above block of instructions have been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is fine to showcase how to change the values. But you can omit the parts that have not changed - e.g.
environment:
- DB_HOST=mongodb://db/openpath_prod_usaid_laos_ev
- ....
- STUDY_CONFIG=usaid-laos-ev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification, I have updated the section showcasing the example of how to change the values for DB_HOST and STUDY_CONFIG, while omitting the parts which have not been changed.
…to crontab folder from em-public dashboard. 2. Added note to update once issue e-mission#1048 is checked in. 3. Removed instructions to use mongodb directly.
…et with changes in docker-compose file.
- Load different datasets, run the scripts to execute the notebooks to make sure the charts are being generate properly. | ||
- Between datasets, drop the old dataset and load the new dataset. | ||
- Also check with empty MongoDB. | ||
3. After the code has been merged, validate the changes on the Staging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fix "the staging"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it to "the staging" since there were other required changes too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iantei this is a nit, but "the staging" is incorrect here. It should be "validate the changes on staging" or "validate the changes on the staging environment".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final fix needed
… code below the description.
…ecution of generate_plots.py script
One final fix |
No description provided.