feat: added wip gradio UI #10
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new Gradio-based UI for the QuantifiedMe project. The implementation includes multiple views for exploring different types of data, such as summary, raw data, correlations, drugs, sleep, time, and data sources. The main.py file contains the core logic for loading data, creating plots, and setting up the Gradio UI, while the main.py file serves as the entry point for running the application. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ErikBjare - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 8 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| from quantifiedme.load.qslang import load_df as load_qslang_df | ||
|
|
||
|
|
||
| @cache |
There was a problem hiding this comment.
suggestion: Consider using lru_cache instead of cache
The cache decorator is a simple alias for lru_cache(maxsize=None). If you anticipate the need to limit the cache size in the future, it might be more flexible to use lru_cache directly.
| @cache | |
| from functools import lru_cache | |
| @lru_cache(maxsize=None) |
| fast = False | ||
| df = load_all(fast=fast) | ||
| print("Slicing timeperiods") | ||
| df = pd.concat([df[(df.index >= start) & (df.index <= end)] for start, end in tps]) |
There was a problem hiding this comment.
issue (performance): Potential performance issue with large dataframes
Using list comprehensions to filter and concatenate dataframes can be inefficient for large datasets. Consider using pd.concat with a generator expression or another method to handle large dataframes more efficiently.
| return df | ||
|
|
||
|
|
||
| def load_timeperiod(period: str) -> pd.DataFrame: |
There was a problem hiding this comment.
suggestion: Function name might be misleading
The function load_timeperiod suggests it loads a single time period, but it actually calls load_timeperiods which handles multiple periods. Consider renaming it to something like load_single_timeperiod for clarity.
| def load_timeperiod(period: str) -> pd.DataFrame: | |
| def load_single_timeperiod(period: str) -> pd.DataFrame: |
|
|
||
| def period_to_timeperiod(period: str) -> tuple[datetime, datetime]: | ||
| now = datetime.now() | ||
| match period.lower(): |
There was a problem hiding this comment.
suggestion: Consider adding a default case for match statement
While the current implementation raises a ValueError for unknown periods, it might be more robust to include a default case in the match statement to handle unexpected inputs gracefully.
| match period.lower(): | |
| match period.lower(): | |
| case "day": | |
| return (now.replace(hour=0, minute=0, second=0), now) | |
| case _: | |
| raise ValueError(f"Unknown period: {period}") |
| raise ValueError(f"Unknown period: {period}") | ||
|
|
||
|
|
||
| def dropdown_dfcols(df) -> gr.Dropdown: |
There was a problem hiding this comment.
suggestion: Type hint for df parameter is missing
Consider adding a type hint for the df parameter to improve code readability and type checking. For example, def dropdown_dfcols(df: pd.DataFrame) -> gr.Dropdown:.
| def dropdown_dfcols(df) -> gr.Dropdown: | |
| import pandas as pd | |
| def dropdown_dfcols(df: pd.DataFrame) -> gr.Dropdown: |
|
|
||
| # when loaded | ||
| # update the top categories plot | ||
| dataframe_el.change( |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with gradio change event
The change event in Gradio might not trigger as expected for DataFrame elements. Ensure that this behavior is tested thoroughly to avoid unexpected issues.
|
|
||
| def view_time(): | ||
| """View to explore time data""" | ||
| pass |
There was a problem hiding this comment.
issue: Unimplemented view functions
The view_time and view_sources functions are currently unimplemented. If these are placeholders, consider adding a TODO comment or raising a NotImplementedError to make it clear that these need to be completed.
| fast = True | ||
| now = datetime.now() | ||
| oldest_start = min((start for start, _ in tps)) | ||
| if oldest_start < now - timedelta(days=30): | ||
| fast = False | ||
| df = load_all(fast=fast) | ||
| print("Slicing timeperiods") | ||
| df = pd.concat([df[(df.index >= start) & (df.index <= end)] for start, end in tps]) | ||
| print("DONE! Sliced", len(df), "rows") | ||
| print(df) | ||
| return df |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move assignment closer to its usage within a block (
move-assign-in-block) - Simplify boolean if expression (
boolean-if-exp-identity) - Move setting of default value for variable into
elsebranch (introduce-default-else) - Replace if statement with if expression (
assign-if-exp) - Remove unnecessary casts to int, str, float or bool (
remove-unnecessary-cast)
| print(f"Col changed to {col}") | ||
| df = df.reset_index().rename(columns={"index": "date"}) | ||
| df = df[["date", col]] | ||
| y_max = max([v for v in df[col] if isinstance(v, (int, float))], default=0) |
There was a problem hiding this comment.
suggestion (code-quality): Replace unneeded comprehension with generator (comprehension-to-generator)
| y_max = max([v for v in df[col] if isinstance(v, (int, float))], default=0) | |
| y_max = max((v for v in df[col] if isinstance(v, (int, float))), default=0) |
| range = gr.Dropdown( | ||
| label="Range", choices=["Day", "Week", "Month", "Year"], value="Month" | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Don't assign to builtin variable range (avoid-builtin-shadow)
Explanation
Python has a number ofbuiltin variables: functions and constants thatform a part of the language, such as
list, getattr, and type(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
Summary by Sourcery
This pull request adds a new Gradio-based user interface for the QuantifiedMe project, enabling users to explore various types of quantified self data through multiple interactive tabs.