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

Text (yaml) report for download #181

Merged
merged 18 commits into from
Dec 3, 2024
Merged

Text (yaml) report for download #181

merged 18 commits into from
Dec 3, 2024

Conversation

mccalluc
Copy link
Contributor

@mccalluc mccalluc commented Nov 22, 2024

For reviewer:

  • Is the template code understandable?
  • Is the generated code in the notebook ok?
  • Is the format of the report ok? (Not expecting you to mind-read hypothetical future users, but anything that should just be fixed now?)

@mccalluc mccalluc marked this pull request as ready for review November 22, 2024 14:54
@ekraffmiller ekraffmiller self-assigned this Dec 2, 2024
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.

For the report, it looks good, but I think it would be helpful for the user if all the imports for the release were listed.
For example, this is the generated report I got:

inputs:
  data: /var/folders/sv/ps25r8d562z0jzv4y4c_yn7c0000gq/T/fileupload-f2gfx9jw/tmppn4cvg9x/0.csv
outputs:
  educ:
    accuracy: 3.375617766595713
    confidence: 0.95
    histogram:
      (-inf, 1]: 33
      (1, 2.5]: 14
      (10, 11.5]: 167
      (11.5, 13]: 254
      (13, 14.5]: 53
      (14.5, 16]: 36
      (2.5, 4]: 57
      (4, 5.5]: 24
      (5.5, 7]: 54
      (7, 8.5]: 47
      (8.5, 10]: 261

I think it would be good to also include, the epsilon, weight, and maybe number of bins? (Although that is shown in the actual result)

@ekraffmiller ekraffmiller assigned mccalluc and unassigned ekraffmiller Dec 2, 2024
mccalluc and others added 6 commits December 2, 2024 17:11
* use original column names in our report

* flatten util

* checkpoint on CSV report; tests not passing

* csv report works

* add a test

* button grid

* factor out button function

* update test to match
@mccalluc
Copy link
Contributor Author

mccalluc commented Dec 3, 2024

I think this adds all the required info, but we're just inserting the values into the generated code: The values also occur earlier in the template, but they aren't stored into variables. Doing that would add a level of generality that would interfere with the clarity of the examples at the top of the notebook, which I think are the priority... but I could be wrong.

@mccalluc mccalluc requested a review from ekraffmiller December 3, 2024 00:21
@mccalluc mccalluc removed their assignment Dec 3, 2024
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.

I think it looks good, I agree that you don't need to change the notebook to put the values into variables.

@mccalluc mccalluc merged commit 23034e9 into main Dec 3, 2024
2 checks passed
@mccalluc mccalluc deleted the 173-text-yaml-report branch December 3, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

"Please wait" while notebook builds? Download Text report Return results from execution of templated code
2 participants