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

Add --keep flag to crf-search subcommand #166

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

t-nil
Copy link
Contributor

@t-nil t-nil commented Jan 1, 2024

For me it's very useful to look at the generated samples for manual verification. This command works on my machine right now.

@alexheretic please tell me if you want the flag refactored into args::Sample. Its probably better that way, right?

@t-nil t-nil force-pushed the crf_search_keep_samples branch 2 times, most recently from 2730ed2 to b1e51e7 Compare January 1, 2024 15:49
@t-nil
Copy link
Contributor Author

t-nil commented Jan 1, 2024

Btw: what is the double purpose of cleaning in main() and sample_encode::run

@alexheretic
Copy link
Owner

Thanks, I 'm ok with crf-search & auto-encode having --keep. It does make we wonder if we should improve the output a bit though, e.g. informing which temp-dir is being kept so users don't have to guess / set one explicitly.

please tell me if you want the flag refactored into args::Sample. Its probably better that way, right?

Yes I think that makes sense.

Btw: what is the double purpose of cleaning in main() and sample_encode::run

Early cleaning keeps storage usage ceiling down. Cleaning at the end is required to handle ctrl-c interrupts & anything left over.

@t-nil t-nil force-pushed the crf_search_keep_samples branch 2 times, most recently from a557b3c to f557d4e Compare January 16, 2024 14:21
@t-nil
Copy link
Contributor Author

t-nil commented Jan 16, 2024

Should be fine now :)

src/main.rs Outdated Show resolved Hide resolved
@alexheretic
Copy link
Owner

Looks good now, just one minor code comment.

Can you pop something in the changelog?

CHANGELOG.md Outdated Show resolved Hide resolved
@alexheretic alexheretic merged commit 5f6f0ed into alexheretic:main Jan 18, 2024
2 checks passed
@alexheretic
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants