-
Notifications
You must be signed in to change notification settings - Fork 6
Add database schemas, convenience functions, and small tutorial #90
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
Conversation
d6cd6d0 to
423a5f8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
==========================================
+ Coverage 99.65% 99.72% +0.06%
==========================================
Files 4 6 +2
Lines 291 359 +68
==========================================
+ Hits 290 358 +68
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/convenience.jl
Outdated
| adaptive_grad::Bool = false, | ||
| ) |
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.
| adaptive_grad::Bool = false, | |
| ) | |
| adaptive_grad::Bool = false, | |
| clustering_kwargs = Dict(), | |
| weight_fitting_kwargs = Dict(), | |
| ) |
| ) |> DataFrame | ||
| split_into_periods!(df; period_duration) |
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.
| ) |> DataFrame | |
| split_into_periods!(df; period_duration) | |
| ) |> DataFrame | |
| combine_periods!(df) | |
| split_into_periods!(df; period_duration) |
| DuckDB.register_data_frame(connection, clustering_result.profiles, "profiles_rep_periods") | ||
| mapping_df = weight_matrix_to_df(clustering_result.weight_matrix) | ||
|
|
||
| prefix = "" |
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.
| prefix = "" | |
| # Below we register the dataframes as `t_<name>`, because we can't directly | |
| # create them in the `database_schema`. | |
| # Afterwards, we copy them to the schema and drop these `t_<name>` views. | |
| prefix = "" |
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.
Please don't forget to include this one. Thanks!
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.
It's included
src/convenience.jl
Outdated
| niters::Int = 100, | ||
| learning_rate::Float64 = 0.001, | ||
| adaptive_grad::Bool = false, |
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.
Move to dict below
test/test-convenience.jl
Outdated
|
|
||
| connection = _new_connection(; profile_names, years, num_timesteps) | ||
|
|
||
| clusters = cluster!(connection, period_duration, num_rps; database_schema) |
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.
| clusters = cluster!(connection, period_duration, num_rps; database_schema) | |
| clusters = cluster!(connection, period_duration, num_rps; database_schema, | |
| clustering_kwargs = Dict(:display = :iter), | |
| weight_fitting_kwargs = Dict( | |
| ) |
|
@greg-neustroev @datejada I think this is updated now. Since it doesn't depend on TulipaIO, it is ready for review. I didn't put much effort into the tutorial, but I decided to leave to have something about the convenience functions. Let me know if you want it removed. |
datejada
left a comment
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.
@abelsiqueira, thanks for the PR and the changes. The tutorial looks great, we need it.
I followed the changes. My comment is about running it twice. I am unsure if the current code allows us to run it twice with the same connection. It looks like it will throw the error that the table already exists. Would you mind commenting on that (and changing it if needed to be able to run it twice)?
Thanks!
|
Hi @datejada, I've added tests running it twice. Only transforming from wide to long was not working for me. Do you have a different experience? |
That was it 😄 Thanks! |
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.
Thanks for the changes. I just left a minor comment so you resolve the comments you had from yesterday. I'm approving it and merging it when it is ready.
|
I included the comment manually in file src/io.jl. Can you confirm that's the change you meant? |
yes, I figured it out afterwards. All your own comments are addressed, so I would close them to avoid confusion 🤷♂️ |
Related issues
Closes #40
Checklist