Skip to content

add vrhyme tool#1600

Merged
SaimMomin12 merged 24 commits intobgruening:masterfrom
Minamehr:Add_vrhyme
May 14, 2025
Merged

add vrhyme tool#1600
SaimMomin12 merged 24 commits intobgruening:masterfrom
Minamehr:Add_vrhyme

Conversation

@Minamehr
Copy link
Copy Markdown
Contributor

@Minamehr Minamehr commented Apr 3, 2025

Hi – This pull request adds the tool vRhyme to the repository.
Thanks!

@bgruening
Copy link
Copy Markdown
Owner

You have 268 files in this PR. Are all files used?

@Minamehr
Copy link
Copy Markdown
Contributor Author

Minamehr commented Apr 3, 2025

You have 268 files in this PR. Are all files used?

The test-data folder has many files due to multiple test cases to check different functions. I’ll reduce it to one or two representative tests. However, even a single test produces around 50 output files — I’ll reduce them as much as possible.

@bgruening
Copy link
Copy Markdown
Owner

Do you check all these generated outputs? If a test generates 50 files to you check/diff all the 50 files? I can not see that in your PR

@Minamehr
Copy link
Copy Markdown
Contributor Author

Minamehr commented Apr 3, 2025

Do you check all these generated outputs? If a test generates 50 files to you check/diff all the 50 files? I can not see that in your PR

Yes, you're right. Some outputs like alternate_bins_membership and alternate_bins_summary are collections with many files (e.g., 19 elements). I checked representative files (4 out of 19). But I’ve removed those outputs for now.

@bgruening
Copy link
Copy Markdown
Owner

You should only remove the file you are NOT checking. The files that are useless in this PR. Also consider to use asserts to check proprieties of files, this way you don't need to store them at all in git. This does not work for all usecases but for many.

@SaimMomin12 SaimMomin12 merged commit 60ec473 into bgruening:master May 14, 2025
11 checks passed
@Minamehr
Copy link
Copy Markdown
Contributor Author

Hi Saim,
Thanks for your effort on the vRhyme PR (#1600). I saw that you pushed some changes and merged it — I really appreciate you jumping in to help.
However, I was still actively working on the wrapper to solve a specific issue, and the dependencies weren’t finalized yet. It hadn’t been fully tested either.
Since it was merged a bit early, I’ll go ahead and open a follow-up PR to clean things up and restore the changes I was working on.

@SaimMomin12
Copy link
Copy Markdown
Collaborator

Hi @Minamehr, Sorry, I didn’t realize you were still actively working on the PR as it hadn’t seen much activity for a couple of weeks and wasn’t in draft, so when Bjoern asked me about it, I thought I could help move it forward by amending changes and merging. Totally fine to open a follow-up PR with the remaining changes. Also, maybe consider marking WIP PRs as draft next time so it’s clearer they’re still in progress. :)

@Minamehr
Copy link
Copy Markdown
Contributor Author

Hey, no worries at all — thanks for the follow-up and the tip! I’ll make sure to mark it as draft next time to avoid confusion. 😊

@Minamehr Minamehr deleted the Add_vrhyme branch May 21, 2025 09:43
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.

3 participants