-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add logging and progress bar for generation of forms #67
Conversation
@@ -48,7 +49,7 @@ def noise_form( | |||
randomness = get_randomness_stream(form.name, seed) | |||
|
|||
noise_configuration = configuration[form.name] | |||
for noise_type in NOISE_TYPES: | |||
for noise_type in tqdm(NOISE_TYPES, desc="Applying noise", unit="type"): |
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.
I love tqdm - it's just so easy. But what is unit="type"
doing?
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.
Oh, it is doing xxx type/s. Hmm...
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.
maybe unit="noise types"
?
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.
The default it "it" so instead of "it/sec" it says "type/sec". I mean "type" to be noise type, but it could be any string we want.
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.
I wanted to keep it short to save on the terminal space. There is already the description of "Applying noise". There should be no ambiguity there...
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.
Two notes:
- We should set the default log level to
INFO
if it is not already at that level. That should be part of this PR - How does this work when there are multiple files being noised (i.e. say there were two decennial census files in the sample data directory rather than 1)? When they run on the full data they will have 334 files. This does not need to be resolved in this PR.
tqdm is currently wrapping the actual noising loop, so each of the 334 files would get their own progress bar one after another since it's done in serial. |
@rmudambi What Steve said. I tried wrapping the file reading with tqdm, too, but that got messy, since the loops are intertwined. I'm still looking into where/how we ought to set the log level. |
b7bab14
to
c3dad05
Compare
Add logging and progress bar for generation of forms
Description
Changes
Testing
Interactive output looks as expected: