Skip to content
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

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Make notebook with plots for columns #152

wants to merge 35 commits into from

Conversation

mccalluc
Copy link
Contributor

@mccalluc mccalluc commented Nov 8, 2024

Dropped 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:

  • This would be a good one to checkout and run end to end. (With the name change, you'll need to run flit install before you can execute dp-wizard.)
  • There is a lot undone, so don't worry about gaps, but if something is just wrong, that would be good to hear about.
  • After this, I want to take some time to clean up the internals, but there won't be big changes in the UI... so if something would be really good to get sorted before the demo next week, that would be good to note.

Base automatically changed from 150-138-analysis-formatting to main November 13, 2024 18:19
@mccalluc mccalluc marked this pull request as ready for review November 15, 2024 03:47
@ekraffmiller ekraffmiller self-assigned this Nov 15, 2024
@ekraffmiller
Copy link
Member

ekraffmiller commented Nov 15, 2024

Looks good, I started from scratch with the new repository and downloaded a notebook with histograms created.
One thing I noticed, if I change the number of bins using the arrows, then the bins appear out of order in the histogram plot. This happens in the app and in the notebook.
For example,

Screenshot 2024-11-15 at 11 32 05 AM

Copy link
Member

@ekraffmiller ekraffmiller left a 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?

@mccalluc
Copy link
Contributor Author

bins appear out of order in the histogram plot

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.

@mccalluc
Copy link
Contributor Author

should I test the generated python script, or is that still a work in progress?

Not really necessary right now. There's going to be more work on these.

@mccalluc mccalluc requested review from ekraffmiller and removed request for ekraffmiller November 15, 2024 18:19
@mccalluc
Copy link
Contributor Author

Going ahead and fixing the sorting...

@mccalluc
Copy link
Contributor Author

Copy link
Member

@ekraffmiller ekraffmiller left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready to Merge
2 participants