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

Replace Math::fequals with new utility function #5070

Open
gf712 opened this issue Jun 17, 2020 · 16 comments
Open

Replace Math::fequals with new utility function #5070

gf712 opened this issue Jun 17, 2020 · 16 comments

Comments

@gf712
Copy link
Member

gf712 commented Jun 17, 2020

We are in the process of dropping Math.h and one of the more commonly used operation there is fequals, which checks if the difference between two values is less than the type's epsilon. The task is to write this as a free function, probably in a new file in the util directory and drop Math::fequals.

@gf712
Copy link
Member Author

gf712 commented Jun 17, 2020

@karlnapf @vigsterkr I am assuming we should also remove fequals_epsilon in env in favour of std::numeric_limits<T>::epsilon?

@karlnapf
Copy link
Member

there are some issues: CMath is stateful and the fequals_epsilonis modified in some testing parts. I am not sure that is still the case (it was basically to decrease accuracy temporarily in the integration testing without having to pass the epsilon down through 10 functions). Given that serialization now works differently (and should be lossless?), the epsilon is never changed. In that case I agree

@5aumy4
Copy link

5aumy4 commented Jul 3, 2020

Hello! I am new to open source contributions.Can I do some help ?

@Raghibshams456
Copy link

is anybody is doing or done something on this issue .?...

@karlnapf
Copy link
Member

karlnapf commented Jul 7, 2020

nope :)

@AvidDrawer
Copy link

I've looked into math.h and the simplest solution is to lift the fequals function there and add it into a separate function/header file. Doing that will account for the stateful-ness of math.h without diving deep to ascertain the current status of the serialization (presumably lossless).

However, this seems way too simple necessitating my question- would that suffice as a solution? If not, could you please point out what/where this serialization is? @karlnapf @gf712

@yuvraj2701
Copy link

Hi! Is anyone working over this issue? I would like to take it up if nobody is.
Thanks!

@AvidDrawer
Copy link

Go ahead. From the looks of it, I don't think anyone else is.

I'm stopped since it looked like a fair bit of work that might end up being unused if I end up in the wrong direction.

@yuvraj2701
Copy link

Thank you
I am a newbie here. If possible could you please give me some direction/hints to help me get started and move forward with the task?

@AvidDrawer
Copy link

I'm not the best person for this. If you search for mathematics.h, you should be able to find the function that we are trying to replace. One potential solution is as I described/queried on the comment above.

@justdvnsh
Copy link

@karlnapf @gf712. Hi, I am new in shogun and want to start contributing. Can I be assigned this issue ?

@yuvraj2701
Copy link

Hey
I am still working over it

@yuvraj2701
Copy link

@karlnapf @gf712 can you please tell whether the other methods of Math.h (which are used by fequals function like abs<> ,fequals_abs, etc.) need to be moved to the same new file or to some other file or they should be kept in Math.h itself?

@gf712
Copy link
Member Author

gf712 commented Oct 9, 2020

Hi, sure, those can be moved as well. Easiest is to start a PR and can take it from there.

@yuvraj2701
Copy link

@gf712 I have started a pr (#5130) .
Not sure about why the tests are failing. Can you please guide me?

@nalinwadhwa02
Copy link

Hello im new here and i would like to start contributing.
Is this thread still open/ not being worked on by anyone ?
If so I would also try to get my fingers wet :)

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

No branches or pull requests

8 participants