Implements rint #58

khurd21 · 2024-09-01T02:00:09Z

Functions implemented:

ccm::rint(float)
ccm::rint(double)
ccm::rint(long double)
ccm::rintl(long double)
ccm::rintf(float)
ccm::lrint(float)
ccm::lrint(double)
ccm::lrint(long double)
ccm::lrintl(long double)
ccm::lrintf(float)
ccm::llrint(float)
ccm::llrint(double)
ccm::llrint(long double)
ccm::llrintl(long double)
ccm::llrintf(long double)

Tests written for:

Common and edge case conditions for float, double, and long double inputs.
Assert that FE_INVALID is raised for +/- infinity values and NaN
values.
Asserts that functions can be evaluated at compile time.

khurd21 · 2024-09-01T02:00:38Z

include/ccmath/math/nearest/rint.hpp

+	}
+
+	template <class Integer, std::enable_if_t<std::is_integral_v<Integer>, bool> = true>
+	constexpr double rint(Integer num)


Add noexcept? Also need to document each function.

Following the defined API of the standard there should not be a noexcept for ccm::rint. This may change later in the future but for the time being aim to mimic the API pretty closely.

You can also use this as a reference:

https://en.cppreference.com/w/cpp/numeric/math/rint

@Rinzii Sounds good. I removed the noexcept for that method!

I think it is ready for a review!

Rinzii · 2024-09-01T02:32:05Z

Glad to see a new PR! I will warn you though. I've recently pushed a major internal restructure of the project to the dev branch so make sure your fork is up to date!

khurd21 · 2024-09-01T19:01:46Z

test/nearest/rint_test.cpp

+	};
+
+	template <typename T>
+	std::vector<T> makeTestParams()


After pushing this all up, I realized I had a question: Is there a specific casing I should be following? I am assuming snake case to match the standard library?

There is no hard rule, but generally yes I follow pretty close to LLVMs style using snake case. You can also use clang-format to aid you in making sure your following the formatting style of the project!

Sounds good, thanks. I set up the clang format in VSCode to run on every save, however it didn't seem to adjust for the snake vs camel case. I will update the casing for this commit! :)

Rinzii

Thank you for taking the time to contribute to CCMath! The work that people like you do is invaluable and I greatly appreciate the time you have put in.

Overall, this is a good start! I have a few minor change requests, but also some major change requests that may be trickier to deal with. If you need help or advice on some of the requests I've made feel free to ping me.

I look forward to your changes!

Rinzii · 2024-09-02T03:47:12Z

test/nearest/rint_test.cpp

+		#define CLANG_LINUX
+	#endif
+#endif
+


Can we also test here that in the events where a fenv error is raised that it is actually being set for run time evaluation? (I should also let you know that the internal helpers we use for fenv will not set fenv if we are being evaluated during compile time (though this may change in the future))

Also I know I've yet to start pushing this onto the test cases yet (but I have been planning for it) but now that many functions are starting to account for rounding modes could we also test that our values are correct when the rounding modes change? If this is too much work I understand, but I'd like to start pushing the code base to properly respect and test rounding modes if possible! (Also you should not need to test this for compile time as during compile time the rounding mode is always round-to-nearest!)

Looking at your other comments, I think I did make an oversight on the rounding modes. I was really only considering the compile time evaluation when I wrote them, so thanks for bringing this up!

I think my plan moving forward for this PR is to sometime this week / weekend, I will write tests that run for each rounding mode and then adapt what I currently have to work for the tests. I might be able to simply use your helper functions for the majority of these besides the rounding functions for long and long long types.

Rinzii · 2024-09-02T04:09:36Z

include/ccmath/math/nearest/rint.hpp

+	constexpr T rint(T num)
+	{
+		if (ccm::isinf(num) || ccm::isnan(num)) { ccm::support::fenv::raise_except_if_required(FE_INVALID); }
+		constexpr auto rounding_mode{ccm::support::fenv::get_rounding_mode()};


This function can't have rounding_mode as constexpr because if the compiler evaluates get_rounding_mode() at compile time, it will lock in a specific rounding mode (e.g., round-to-nearest) regardless of the actual rounding mode at runtime. This would lead to incorrect rounding behavior when the function is called at runtime since the rounding mode may differ from the one determined at compile time. So I'd remove the constexpr qualifier here.

Of course you can change this to const to take advantage of constant folding!

Rinzii · 2024-09-02T04:28:19Z

include/ccmath/math/nearest/rint.hpp

+	 */
+	constexpr long lrint(float num)
+	{
+		return static_cast<long>(ccm::rint(num));


Ok, this function here is a bit trickier and performing a static_cast won't actually be enough to respect all rounding modes. static_cast does not perform conversion with floats but instead performs truncation and this will always follow the default rounding mode. As such if the rounding mode is different than the default this should not work as expected. The proper solution is to instead get the bits of the float itself and perform rounding towards a signed integer using the bits themselve. There is a few manners to do this, but it will be a tricky problem to fix. Let me know your thoughts!

I will have to investigate this after writing the tests I think, cause I initially assumed this would be an OK cast to perform since the long and float are the same size? I will have to brush up on my casting :)

Thanks for pointing this out!

Rinzii · 2024-09-02T04:29:22Z

include/ccmath/math/nearest/rint.hpp

+	 */
+	constexpr long lrint(double num)
+	{
+		if (ccm::isnan(num))


This will also suffer from the issue I mentioned earlier with static_cast along with every other lrint function that converts a float to a integer.

khurd21 · 2024-09-04T04:55:05Z

@Rinzii

I noticed two things from your comments that seem the most important. I wanted to write them down here because I don't think I addressed these in an earlier PR for implementing ccm::nearbyint.

constexpr keyword being used to get the rounding mode: I used constexpr for ccm::nearbyint implementation.
Testing rounding mode type at runtime. I did not add these for nearbyint, however it does look like adding them just for rint would be testing the same behaviors.

Should I also re-address these two things there? I think for this PR, I could change constexpr to const in the ccm::nearbyint and leave it at that, however it makes sense to test the rounding modes in that function as well as it would have caught this potential bug in getting the rounding mode at runtime.

Thanks for the review!

Functions implemented: - ccm::rint(float) - ccm::rint(double) - ccm::rint(long double) - ccm::rintl(long double) - ccm::rintf(float) - ccm::lrint(float) - ccm::lrint(double) - ccm::lrint(long double) - ccm::lrintl(long double) - ccm::lrintf(float) - ccm::llrint(float) - ccm::llrint(double) - ccm::llrint(long double) - ccm::llrintl(long double) - ccm::llrintf(long double) Tests written for: - Common and edge case conditions for `float`, `double`, and `long double` inputs. - Assert that FE_INVALID is raised for +/- infinity values and NaN values. - Asserts that functions can be evaluated at compile time.

khurd21 · 2024-09-16T03:02:52Z

@Rinzii

I tried to address a few things in my recent changes:

Removed the constexpr when getting the rounding mode to prevent compile time evaluation.
Added a check for when FE_INVALID is raised to assert that it is being performed at runtime. To do this, I used ccm::support::is_constant_evaluated. Please do let me know if this was not was you were suggesting!

For addressing the different rounding modes, I was struggling to find a clean way to test them all without having a ton of duplicate code. I ended up making a decision to have cmake generate source files for us, changing the rounding mode for each source file generated. The end result is four generated files to test: DOWNWARD, TONEAREST, TOWARDZERO, and UPWARD. They all use the same template rint_test.cpp.in. This also forced me to separate out the tests that test for compile time evaluation. These are now in: rint_test_compile_time.cpp.

Here is an example of what they would look like in an IDE. The files are placed in an out folder for the specific compiler in use.

In doing so, I also found that the static_cast for the llrint, lrint etc functions seem to be a non-issue. The rounding for the floating point values are already performed before the static_cast in performed, so because they are representing a whole number at that point, a static cast should not have an adverse effect on the number.

I did notice though that msvc handles max and min values slightly differently for llrint and lrint functions. If it is the maximum or minimum, it returns 0. Clang and gcc did not seem to be implemented this way. If this sounds OK to you, I was considering adding a check to return 0 for MSVC given these conditions.

It also sounded like I may be going a different route with handling the rounding modes for the llrint-like functions, so I wanted to check in with your opinion before going much farther!

Thanks, and sorry about the slower responses on the PR!

Rinzii · 2024-09-20T04:35:21Z

Using cmake to generate different test cases is perfectly fine.

Also adding a msvc specific return is perfectly fine and actually encouraged as ccmath tends to aim to follow the expected output of each compiler!

Also don't worry about taking your time with the PR. I'd rather you took the time to get everything right than rushing stuff!

If you have any specific questions for me just lmk! I've been pretty busy these last few weeks, but I'll try to get back to any questions you have promptly.

khurd21 commented Sep 1, 2024

View reviewed changes

khurd21 force-pushed the 7-rint branch 4 times, most recently from a19e9e7 to 719e2dd Compare September 1, 2024 18:50

khurd21 changed the title ~~(WIP): Implements rint~~ Implements rint Sep 1, 2024

khurd21 marked this pull request as ready for review September 1, 2024 18:58

khurd21 commented Sep 1, 2024

View reviewed changes

Rinzii requested changes Sep 2, 2024

View reviewed changes

Rinzii assigned khurd21 Sep 2, 2024

Rinzii added the enhancement New feature or request label Sep 2, 2024

Rinzii added this to the Road to v1.0.0 milestone Sep 2, 2024

khurd21 force-pushed the 7-rint branch from 719e2dd to afe1c39 Compare September 15, 2024 03:46

khurd21 force-pushed the 7-rint branch from afe1c39 to fd06b4f Compare September 15, 2024 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements rint #58

Implements rint #58

khurd21 commented Sep 1, 2024 •

edited

Loading

khurd21 Sep 1, 2024

Rinzii Sep 1, 2024

Rinzii Sep 1, 2024

khurd21 Sep 1, 2024

Rinzii commented Sep 1, 2024

khurd21 Sep 1, 2024

Rinzii Sep 2, 2024

khurd21 Sep 4, 2024

Rinzii left a comment

Rinzii Sep 2, 2024

Rinzii Sep 2, 2024

khurd21 Sep 4, 2024

Rinzii Sep 2, 2024

Rinzii Sep 2, 2024

Rinzii Sep 2, 2024

khurd21 Sep 4, 2024

Rinzii Sep 2, 2024

khurd21 commented Sep 4, 2024 •

edited

Loading

khurd21 commented Sep 16, 2024 •

edited

Loading

Rinzii commented Sep 20, 2024 •

edited

Loading

Implements rint #58

Are you sure you want to change the base?

Implements rint #58

Conversation

khurd21 commented Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rinzii commented Sep 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rinzii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khurd21 commented Sep 4, 2024 • edited Loading

khurd21 commented Sep 16, 2024 • edited Loading

Rinzii commented Sep 20, 2024 • edited Loading

khurd21 commented Sep 1, 2024 •

edited

Loading

khurd21 commented Sep 4, 2024 •

edited

Loading

khurd21 commented Sep 16, 2024 •

edited

Loading

Rinzii commented Sep 20, 2024 •

edited

Loading