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

Update rand with C++11 random generators in tests module #3867

Conversation

shrijitsingh99
Copy link
Contributor

Solves rand uses in tests module as mentioned in #3781
Modules left in tests:

  • common
  • filters
  • io
  • kdtree
  • outofcore
  • registration
  • sample_consensus
  • search
  • segmentation

@shrijitsingh99
Copy link
Contributor Author

@kunaltyagi Before I go ahead and implement this for the remaining modules. Is the any problem or changes needed in the current implementation?

@SergioRAgostinho
Copy link
Member

Word of advice for this PR: split changes which make unit test fail into isolated PRs. They usually involve a lot of discussion and considerable changes on their own.

@shrijitsingh99
Copy link
Contributor Author

Word of advice for this PR: split changes which make unit test fail into isolated PRs. They usually involve a lot of discussion and considerable changes on their own.

So do you mean split into separate PR's based on modules?

@SergioRAgostinho
Copy link
Member

Not necessarily. Make all the changes that you can, which do not require you to touch the unit tests. We merge those first. Then we open a PR for each change which requires modifying the unit tests, because those tend to be noisy PRs. They usually involve digging around the reason the unit test is failing, occasionally rewriting unit tests from scratch to actually do sane testing.

As an example have a look at how I split the commits for the following PR #3862. I made all the changes I could until the point I knew it would break the unit tests. The followup commits are to address the broken unit test. Everything is in a single PR because I was only touching a single class.

@kunaltyagi
Copy link
Member

kunaltyagi commented Apr 18, 2020

Rebase and push to trigger another CI run. As per Ubuntu, there seems to be no issues, but let's see with the latest master once again

The API seems awkward, but that's upto another PR to make it better (or replace/remove it) (discussion issue created)

Usage is correct and I'd recommend moving forwards, specially with the cloud generators. The API update seems the only reason I'd consider for not moving ahead.

@shrijitsingh99
Copy link
Contributor Author

Not necessarily. Make all the changes that you can, which do not require you to touch the unit tests. We merge those first. Then we open a PR for each change which requires modifying the unit tests, because those tend to be noisy PRs. They usually involve digging around the reason the unit test is failing, occasionally rewriting unit tests from scratch to actually do sane testing.

Got it thanks 👍 . Will keep this in mind in the future and will modify this PR accordingly.

Rebase and push to trigger another CI run. As per Ubuntu, there seems to be no issues, but let's see with the latest master once again

Done

Usage is correct and I'd recommend moving forwards, specially with the cloud generators. The API update seems the only reason I'd consider for not moving ahead.

Yeah will defer this until API update no point doing a refactor again if there is some major API change and making some modifications to the API will definitely make it cleaner.

@stale
Copy link

stale bot commented May 19, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label May 19, 2020
@shrijitsingh99
Copy link
Contributor Author

Closing this will pick it up again once we have the new random interface as being discussed #3996

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