Issues in the reporting code identified during the initial move to AO

bechandcock commented 4 years ago

[ ] Resolve issue of hard coding of asset filenames
[ ] Add or check error-handling for if the supplied image assets do not exist.
[ ] Check tests for the Charting functionality for any requested changes since the move
[ ] Develop tests for the Reporting functionally
[ ] Check if “get_data" can be removed from the JSON and/or report_analytics.py as it might be redundant/duplicated elsewhere in the repository setup files
[ ] Check how the comparison institution lists are extracted and if this is still stable following the code move.
[ ] Check consistency of naming for “plot_graphs” and “do_analysis” between different modules
[ ] Check consistency of naming for calling the precipy function “batch” and “af” between different modules as they are used interchangeably.
[ ] Check how “comparison” is used, passed as a variable and if it is used or not. I have a comment that “comparison is passed as a variable but not currently used I think. You could comment out line 129 and replace comparison_group with comparison throughout I think (or just rename the incoming variable)”
[ ] Check the consistency of file naming conventions in the charts folder. Both using underscores between lower case words.
[ ] Check if the following suggestion was fully addressed in the initial move phase: “Charts folder … considering whether it will be one file per chart, or one file for all charts. Currently some are in the charts.py, some are one chart per file, and other contain multiple charts in one file. I'm leaning towards one per file, just because the implementation of each chart is a little long (and varied) so it might make reading them easier.”
[ ]
[ ] Check whether the locations of palettes and output_types in ‘defaults.py’ an ‘charts_utils.py’need to be tidied up
[ ] Remove any in-line comments which were temporarily added as part of the initial big code move
[ ] Consider whether the way the Reports handles BIgQuery data sites with how it is managed in the rest of the academic-observatory (will need input from Richard/Jamie on this). E.g. see function ‘get_gcp_credentials’

bechandcock commented 4 years ago

Additional issues extracted from TODOs:

~reports/charts/bar_comparison_chart.py

[ ] BarComparisonChart there was a TODO# Greater flexibility for color palettes and options for other variations on OA.
[ ] process_data there was a TODO# Current hardcodes the location of uni ids and the year in the dataframe. Generalise this and allow for more flexibility of data types
[ ] plot there was a TODO# Currently hardcodes the kinds of OA to be plotted. This should be abstracted to allow greater flexibility.

~reports/charts/confidence_interval_rank.py
[ ] process_data there was a TODO# Abstract the coloring

~reports/charts/funder_count_bar_graph.py
[ ] process_data there was a TODO# mapping of nicely formatted funder names

~reports/charts/funder_graph.py
[ ] process_data there was a TODO# Shorten funder names to avoid an issue that can arise where the length of the axis labels leads to a matplotlib error
[ ] process_data there was a TODO# when the figure is drawn ValueError: left cannot be >= right
[ ] process_data there was a TODO# mapping of nicely formatted funder names

~reports/charts/rank_chart.py
[ ] process_data there was a TODO# Abstraction of the coloring for the error bars
[ ] process_data there was a TODO# Abstract the coloring
[ ] scale there was a TODO# Figure out if this can be done more cleanly with matplotlib transform methods.

~reports/charts/scatter_plot.py
[ ] process_data there was a TODO# Abstract the ordering and colors for better flexibility
[ ] animate there was a TODO# Generalise the output form to allow for JS and other representations of the animation.

~reports/abstract_chart.py
[ ] _check_df there was a TODO# Some error checking on df being in right form

~reports/chart_utils.py
[ ] clean_geo_names there was a #TODO: cleanup required, mainly on country names
[ ] calculate_confidence_interval there was a TODO# Check that this calculation is up to date with our standard practise

bechandcock commented 4 years ago

All of these TODOs and issues need to be moved/grouped on individual issue/tickets

bechandcock commented 3 years ago

I am assigning this to @cameronneylon to confirm closing this issue.

cameronneylon commented 3 years ago

This remains a good list of things to work through so I'll leave this open for the moment and resolved in the next push on code clean up and pull request

The-Academic-Observatory / observatory-reports

Issues in the reporting code identified during the initial move to AO #4

[ ] Develop tests for the Reporting functionally

[ ] Remove any in-line comments which were temporarily added as part of the initial big code move

Additional issues extracted from TODOs:

[ ] plot there was a TODO# Currently hardcodes the kinds of OA to be plotted. This should be abstracted to allow greater flexibility.

[ ] process_data there was a TODO# Abstract the coloring

[ ] process_data there was a TODO# mapping of nicely formatted funder names

[ ] process_data there was a TODO# mapping of nicely formatted funder names

[ ] scale there was a TODO# Figure out if this can be done more cleanly with matplotlib transform methods.

[ ] animate there was a TODO# Generalise the output form to allow for JS and other representations of the animation.

[ ] _check_df there was a TODO# Some error checking on df being in right form