-
Notifications
You must be signed in to change notification settings - Fork 1
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
Make notebook with plots for columns #152
base: main
Are you sure you want to change the base?
Conversation
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.
Looks good, I just noticed that histogram bins are sometimes out of order, see below. For this PR, should I test the generated python script, or is that still a work in progress?
Good catch: These strings are being sorted alphabetically, so "10" comes before "2". I think the best way to fix this is to pull out the first number of a pair and sort, probably on the python side, rather than trying to parse the string inside polars. Will file a new issue. |
Not really necessary right now. There's going to be more work on these. |
Going ahead and fixing the sorting... |
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.
looks good! bins sorting correctly
utf8-lossy
in templated code #42Dropped the expected-script test: when it was short, it was useful, but now it just means that every change requires a corresponding update to the test: Better to have finer grained unit tests that catch problems earlier.
In the notebook template, added lots of comment blocks that will be turned into markdown cells. The
# +
and# -
are the jupytext light format.This is a change, but I think that having the executed notebook be the real release from the perspective of this application makes sense: That's the best way of making sure the code in the notebook does what we think it should do.
For reviewer:
flit install
before you can executedp-wizard
.)