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.

Sign up for GitHub

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

Jump to bottom

drop CMath class #4186

Open
karlnapf opened this issue Feb 23, 2018 · 3 comments
Open

drop CMath class #4186

karlnapf opened this issue Feb 23, 2018 · 3 comments
Labels
good first issue Tag: Cleanup

Comments

@karlnapf
Copy link
Member

Large entrance task. The goal is to stop using CMath in the shogun codebase and use the C++ standard library instead, e.g. std::log rather than CMath::log or std::sort rather than CMath::qsort.

Once a method is not used anymore, it should be dropped from the CMath class entirely.

PRs should be sent method by method pls.

Good entrance task if you want to work on modernizing shogun
@Yashs744 Yashs744 mentioned this issue Feb 25, 2018
Merged
@Yashs744
Copy link
Contributor

Hello,
I tried doing this and updated DynArray.h but when I sent PR, it didn't pass the first test and I'm not getting what went wrong. Can you help?

@shubham808 shubham808 mentioned this issue Feb 28, 2018
Merged
@shubham808
Copy link
Contributor

shubham808 commented Feb 28, 2018 •
edited

@karlnapf I am ready with a PR to get rid of CMath::log but the gpl submodule needs to be updated as well..... should i make a PR against shogun-gpl ? (Its causing build to break on Travis since CMath::log does not exist anymore)

@karlnapf
Copy link
Member Author

karlnapf commented Mar 1, 2018

Yes, exactly

This was referenced Mar 3, 2018
Merged
Merged
sunalbert added a commit to sunalbert/shogun that referenced this issue Mar 22, 2018
@sunalbert
Fix issue shogun-toolbox#4186, drop CMath::is_infinity
8025ffc
@sunalbert sunalbert mentioned this issue Mar 22, 2018
Merged
karlnapf pushed a commit that referenced this issue Apr 2, 2018
@sunalbert @karlnapf
Fix issue #4186, replace CMath::is_infinity with std::isinf (#4213)
bccd76c 
* Fix issue #4186, drop CMath::is_infinity

* delete the is_infinity function in math.h

* update src/gpl
@syashakash syashakash mentioned this issue Apr 7, 2018
Merged
ktiefe pushed a commit to ktiefe/shogun that referenced this issue Jul 30, 2019
@sunalbert @ktiefe
Fix issue shogun-toolbox#4186, replace CMath::is_infinity with std::i…
56ada54 
…sinf (shogun-toolbox#4213)

* Fix issue shogun-toolbox#4186, drop CMath::is_infinity

* delete the is_infinity function in math.h

* update src/gpl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Tag: Cleanup
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@karlnapf @Yashs744 @shubham808