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

Refactoring by splitting code into multiple files #53

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

jyliuu
Copy link
Collaborator

@jyliuu jyliuu commented Dec 17, 2023

randomPlantedForests.cpp is now splitted into different files which are located in the include and lib directories under src. They contain the headers and class implementations respectively. In particular, the implementation for the classification rpf is now separate from rpf implementation used for regression.

Copy link
Collaborator

@jemus42 jemus42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

Everything appears to still work as expected on the R side, aside from one test that I can't reproduce failing locally.

Only concern I have is that WARNING about the non-portable Makefile.

src/Makevars Show resolved Hide resolved
@jyliuu jyliuu requested a review from jemus42 December 19, 2023 14:22
@jyliuu
Copy link
Collaborator Author

jyliuu commented Dec 19, 2023

@jemus42 the portability issue seems to be fixed, but not sure about the test fail. I don't think that is due to my change.

@jyliuu
Copy link
Collaborator Author

jyliuu commented Dec 19, 2023

@jemus42 the portability issue seems to be fixed, but not sure about the test fail. I don't think that is due to my change.

Oddly enough, it seems that the test only fails for ubuntu. Perhaps this has something to do with the RNG?

@jemus42
Copy link
Collaborator

jemus42 commented Dec 20, 2023

@jemus42 the portability issue seems to be fixed, but not sure about the test fail. I don't think that is due to my change.

Oddly enough, it seems that the test only fails for ubuntu. Perhaps this has something to do with the RNG?

Yes, I'll fix and/or remove that test after merging your PR - I don't see any way your changed would be related to that, and even that, that test is... not very sound in general 🥴

Otherwise I think this is good to go, thanks a lot!
I'll shamelessly ignore Marvin and Munir and just merge this 😬

@jemus42 jemus42 merged commit 8d78ca0 into PlantedML:master Dec 20, 2023
3 of 7 checks passed
@jyliuu jyliuu deleted the refactoring-splitting branch December 20, 2023 12:22
@mnwright
Copy link
Collaborator

Looks great!

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

3 participants