AnnMarieW / dash-app-gallery

Dash Examples Index
https://dash-example-index.herokuapp.com/
MIT License
64 stars 25 forks source link

Updating keyfiguresfinland #125

Closed tuopouk closed 1 year ago

tuopouk commented 1 year ago

I should not have included the clock now.

Coding-with-Adam commented 1 year ago

Thank you @tuopouk @AnnMarieW would you be able to do a code review for this next week?

AnnMarieW commented 1 year ago

Yes, I can make some time next week.

tuopouk commented 1 year ago

@AnnMarieW Here is also the serverside version. You can compare the map updates. To me it seems that the clientside version's update is faster. I have a some demos here: Clientside callback demo (more data): http://keyfiguresfinland.herokuapp.com/ Serveside callback demo: https://serversidefigures.herokuapp.com/

AnnMarieW commented 1 year ago

This is a great topic - nice to see an app with data from a country other than US or Canada. I like the use of the clientside callback, it's a significant performance bump -- and the figure is fantastic.

For the review, let's start by focusing on this callback:

@app.callback(Output('key-figures-finland-whole-country-header-x','children'),Input('key-figures-finland-key-figure-selection-x', 'value'))
def update_whole_country_header(key_figure):
    kf_string = key_figure.split(',')
    kf_string.pop(-2)
    kf_string = ', '.join(kf_string)
    kf_string = {True:key_figure, False:kf_string}[len(kf_string.split(','))==1]
    return html.P([kf_string,html.Br(),"in Finland:",html.Br(), ("{:,}".format(whole_country_df.loc[key_figure])).replace('.0','').replace(',',' ')+key_figure.split(',')[-2].replace('2020','').replace('2021','').replace(key_figure.split(',')[0],'')],className="fw-bold")

It's difficult to understand what's going on in the callback function. After a bit of study, it appears that you are parsing the valueof the Dropdown and displaying it as a sentence that includes the value from the whole_country_df

This needs to be simplified to make the code more readable and maintainable. One suggestion is to do a little more data wrangling at the start to make the whole_country_df shaped like this:

(This example is just 2 rows to show the format)

data = {
    "stat_key": [
        "Employment rate, %, 2020",
        "Annual contribution margin, EUR per capita, 2020",
    ],
    "stat_name": ["Employment rate", "Annual contribution margin"],
    "units": ["%", "EUR per capita"],
    "year": ["2020", "2020"],
    "stat": [69.5, 728.8],
}
whole_country_df = pd.DataFrame(data).set_index("stat_key")

Then the callback could look like:

@app.callback(
    Output("key-figures-finland-whole-country-header-x", "children"),
    Input("key-figures-finland-key-figure-selection-x", "value"),
)
def update_whole_country_header(key_figure):
    dff = whole_country_df.loc[key_figure]
    return html.Div(
        [
            html.Div([dff.stat_name, ", ", dff.year]),
            html.Div("in Finland"),
            html.Span([dff.stat, dff.units]),
        ]
    )

Now it's much easier to see what this callback does. Plus, if you would like to style the returned data in a different way, like put it in a card, or make the number stand out more, then it will be really easy to update.

tuopouk commented 1 year ago

@AnnMarieW I updated the code making it probably easier to read. The challenges were to extract names, units and years from the stat name, to display discrete values (e.g. population) as integer values, and to have a space as the thousand separator. I hope it makes more sense now. I also formatted the dropdown style because the labels overlapped on mobile.

AnnMarieW commented 1 year ago

Hi @tuopouk

Yes, the latest changes are much easier to read!

The formatting of the number can be done with the other data wrangling in the global space rather than the callback. Personally, I think the values would look nice with one decimal place and comma separators, and this could be done with an f-string in the component. This is a style question, and Adam can weigh in too - it's a trade-off but we are trying to keep the code simple and concise for the Example Index.

I recently made a checklist for all the things we look for in the code review. Can you please review the list and try to complete as many as possible?

Code Review Checklist

Basic Requirements

Naming conventions

Concise code

In apps using dash-bootstrap-components:

Before release: (For Maintainers only)