Feature/duckdb #25
Merged
Feature/duckdb #25
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Reviewer's GuideAdds a new DuckDB-based patient data analysis example, wires it into the source-code docs, and updates workshop prerequisites to mention environment and access requirements. Sequence diagram for new-style lazy DuckDB high fever querysequenceDiagram
actor Analyst
participant Notebook
participant DuckDBPyConnection
participant DuckDBPyRelation
participant DuckDBEngine
Analyst->>Notebook: Run_cell_create_connection
Notebook->>DuckDBPyConnection: connect(database_path)
DuckDBPyConnection-->>Notebook: connection_instance
Analyst->>Notebook: Run_cell_build_lazy_query
Notebook->>DuckDBPyConnection: sql(SELECT patient, temperature FROM patient_experiment)
DuckDBPyConnection-->>Notebook: DuckDBPyRelation
Notebook->>DuckDBPyRelation: filter(temperature > 39_5)
DuckDBPyRelation-->>Notebook: DuckDBPyRelation
Notebook->>DuckDBPyRelation: aggregate(patient, COUNT(temperature) AS high_fever_count, MAX(temperature) AS max_temperature, group_expr=patient)
DuckDBPyRelation-->>Notebook: DuckDBPyRelation
Analyst->>Notebook: Run_show_on_relation
Notebook->>DuckDBPyRelation: show()
DuckDBPyRelation->>DuckDBEngine: execute_lazy_plan
DuckDBEngine-->>DuckDBPyRelation: result_rows
DuckDBPyRelation-->>Notebook: rendered_table
Notebook-->>Analyst: Display_high_fever_counts
Entity relationship diagram for DuckDB patient analysis views and tableserDiagram
patient_experiment {
BIGINT patient
TIMESTAMP date
DOUBLE temperature
DOUBLE dose
}
patient_metadata {
BIGINT patient
VARCHAR condition
VARCHAR gender
VARCHAR filename
}
hypothesis {
BIGINT patient
DOUBLE max_temperature
DOUBLE total_dose
}
time_series {
TIMESTAMP date
DOUBLE temperature_1
DOUBLE dose_1
DOUBLE temperature_2
DOUBLE dose_2
DOUBLE temperature_3
DOUBLE dose_3
DOUBLE temperature_4
DOUBLE dose_4
DOUBLE temperature_5
DOUBLE dose_5
DOUBLE temperature_6
DOUBLE dose_6
DOUBLE temperature_7
DOUBLE dose_7
DOUBLE temperature_8
DOUBLE dose_8
DOUBLE temperature_9
DOUBLE dose_9
}
patient_experiment ||--|| hypothesis : aggregates_to
patient_experiment ||--|| time_series : pivoted_by_date
patient_experiment ||--|| patient_metadata : shares_patient_id
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In source-code/README.md the DuckDB section is listed twice (and one bullet has a typo:
Kllustrations); consider consolidating these into a single DuckDB bullet with consistent wording. - The notebook currently uses hard-coded relative paths like
'data/patient_experiment.csv'and'data/patient_metadata.csv'; consider resolving the data directory viapathlibrelative to the notebook file to make it more robust when run from different working directories. - The call
duckdb.connect('data/patient_experiment.csv')overloads the concept of a database file and the CSV input; it may be clearer to connect to a.duckdbdatabase file (e.g.,duckdb.connect('patients.duckdb')) and then read the CSVs viaread_csv_autointo views or tables.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In source-code/README.md the DuckDB section is listed twice (and one bullet has a typo: `Kllustrations`); consider consolidating these into a single DuckDB bullet with consistent wording.
- The notebook currently uses hard-coded relative paths like `'data/patient_experiment.csv'` and `'data/patient_metadata.csv'`; consider resolving the data directory via `pathlib` relative to the notebook file to make it more robust when run from different working directories.
- The call `duckdb.connect('data/patient_experiment.csv')` overloads the concept of a database file and the CSV input; it may be clearer to connect to a `.duckdb` database file (e.g., `duckdb.connect('patients.duckdb')`) and then read the CSVs via `read_csv_auto` into views or tables.
## Individual Comments
### Comment 1
<location> `source-code/duckdb/patients.ipynb:473-480` </location>
<code_context>
+ "outputs": [],
+ "source": [
+ "conn.execute('''\n",
+ " CREATE VIEW hypothesis AS SELECT\n",
+ " patient,\n",
+ " MAX(temperature) AS 'max_temperature',\n",
+ " SUM(dose) AS 'total_dose'\n",
+ " FROM patient_experiment\n",
+ " GROUP BY patient\n",
+ " ORDER BY max_temperature DESC;\n",
+ "''');"
+ ]
</code_context>
<issue_to_address>
**suggestion:** Avoid including ORDER BY inside the hypothesis view, since ordering is not guaranteed when selecting from a view.
Here, the ORDER BY has no effect on queries like `SELECT MAX(total_dose) FROM hypothesis`, and it may mislead future readers into thinking the view guarantees sorted output. Please remove the ORDER BY from the view definition and apply ordering only in the outer queries that need it.
```suggestion
" CREATE VIEW hypothesis AS SELECT\n",
" patient,\n",
" MAX(temperature) AS 'max_temperature',\n",
" SUM(dose) AS 'total_dose'\n",
" FROM patient_experiment\n",
" GROUP BY patient;\n",
"''');"
```
</issue_to_address>
### Comment 2
<location> `docs/README.md:57-59` </location>
<code_context>
be familiar with these as well.
+For following along hands-on, you need
+* laptop or desktop with internet access.
+* a system set up so you can connect to an HPC system, an account on an HPC
+ system (e.g., VSC, CECI, ...), compute credits if that is required to run
</code_context>
<issue_to_address>
**nitpick (typo):** Add an article to make the bullet grammatically complete.
For example, update this bullet to "* a laptop or desktop with internet access." so it matches the others that start with "a".
```suggestion
For following along hands-on, you need
* a laptop or desktop with internet access.
* a system set up so you can connect to an HPC system, an account on an HPC
```
</issue_to_address>
### Comment 3
<location> `docs/README.md:59-61` </location>
<code_context>
+For following along hands-on, you need
+* laptop or desktop with internet access.
+* a system set up so you can connect to an HPC system, an account on an HPC
+ system (e.g., VSC, CECI, ...), compute credits if that is required to run
+ jobs on the HPC system if you want to use an HPC system;
+* a Python environment that can run Jupyter Lab if you want to use your own system;
+* access to Google Colaboratory if you prefer not to install software.
</code_context>
<issue_to_address>
**suggestion (typo):** Improve readability of the long HPC-related bullet point.
The list in this bullet reads a bit awkwardly, especially around "system (e.g., VSC, CECI, ...), compute credits". Consider adding an "and" before "compute credits" or otherwise restructuring to make the list of requirements clearer.
```suggestion
* a system set up so you can connect to an HPC system, an account on an HPC
system (e.g., VSC, CECI, ...) and compute credits if required to run
jobs on the HPC system;
```
</issue_to_address>
### Comment 4
<location> `source-code/README.md:17` </location>
<code_context>
representation and algorithms.
* [`pandas`](pandas): illustrations of using pandas and seaborn.
* [`polars`](polars): Kllustrations of using polars.
+* [`duckdb`](duckdb): illustrations of using DuckDB for SQL queries.
* [`regexes`](regexes): illustrations of using regular expressions for
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo in "Kllustrations".
Change "Kllustrations" to "Illustrations".
```suggestion
* [`polars`](polars): illustrations of using polars.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+473
to
+480
| " CREATE VIEW hypothesis AS SELECT\n", | ||
| " patient,\n", | ||
| " MAX(temperature) AS 'max_temperature',\n", | ||
| " SUM(dose) AS 'total_dose'\n", | ||
| " FROM patient_experiment\n", | ||
| " GROUP BY patient\n", | ||
| " ORDER BY max_temperature DESC;\n", | ||
| "''');" |
There was a problem hiding this comment.
suggestion: Avoid including ORDER BY inside the hypothesis view, since ordering is not guaranteed when selecting from a view.
Here, the ORDER BY has no effect on queries like SELECT MAX(total_dose) FROM hypothesis, and it may mislead future readers into thinking the view guarantees sorted output. Please remove the ORDER BY from the view definition and apply ordering only in the outer queries that need it.
Suggested change
| " CREATE VIEW hypothesis AS SELECT\n", | |
| " patient,\n", | |
| " MAX(temperature) AS 'max_temperature',\n", | |
| " SUM(dose) AS 'total_dose'\n", | |
| " FROM patient_experiment\n", | |
| " GROUP BY patient\n", | |
| " ORDER BY max_temperature DESC;\n", | |
| "''');" | |
| " CREATE VIEW hypothesis AS SELECT\n", | |
| " patient,\n", | |
| " MAX(temperature) AS 'max_temperature',\n", | |
| " SUM(dose) AS 'total_dose'\n", | |
| " FROM patient_experiment\n", | |
| " GROUP BY patient;\n", | |
| "''');" |
Comment on lines
+57
to
+59
| For following along hands-on, you need | ||
| * laptop or desktop with internet access. | ||
| * a system set up so you can connect to an HPC system, an account on an HPC |
There was a problem hiding this comment.
nitpick (typo): Add an article to make the bullet grammatically complete.
For example, update this bullet to "* a laptop or desktop with internet access." so it matches the others that start with "a".
Suggested change
| For following along hands-on, you need | |
| * laptop or desktop with internet access. | |
| * a system set up so you can connect to an HPC system, an account on an HPC | |
| For following along hands-on, you need | |
| * a laptop or desktop with internet access. | |
| * a system set up so you can connect to an HPC system, an account on an HPC |
Comment on lines
+59
to
+61
| * a system set up so you can connect to an HPC system, an account on an HPC | ||
| system (e.g., VSC, CECI, ...), compute credits if that is required to run | ||
| jobs on the HPC system if you want to use an HPC system; |
There was a problem hiding this comment.
suggestion (typo): Improve readability of the long HPC-related bullet point.
The list in this bullet reads a bit awkwardly, especially around "system (e.g., VSC, CECI, ...), compute credits". Consider adding an "and" before "compute credits" or otherwise restructuring to make the list of requirements clearer.
Suggested change
| * a system set up so you can connect to an HPC system, an account on an HPC | |
| system (e.g., VSC, CECI, ...), compute credits if that is required to run | |
| jobs on the HPC system if you want to use an HPC system; | |
| * a system set up so you can connect to an HPC system, an account on an HPC | |
| system (e.g., VSC, CECI, ...) and compute credits if required to run | |
| jobs on the HPC system; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a new DuckDB-based patient data analysis example and update documentation for workshop prerequisites and available code examples.
New Features:
Documentation: