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

Consider tightening use of floating-point numbers using custom math types #193

Closed
hannobraun opened this issue Feb 16, 2022 · 5 comments · Fixed by #214 or #215
Closed

Consider tightening use of floating-point numbers using custom math types #193

hannobraun opened this issue Feb 16, 2022 · 5 comments · Fixed by #214 or #215
Labels
topic: core Issues relating to core geometry, operations, algorithms type: development Work to ease development or maintenance, without direct effect on features or bugs

Comments

@hannobraun
Copy link
Owner

Floating-point numbers are a pain. Not only do they have accuracy issues (which are not within the scope of this issue), they also can have weird values, like NaN (Not a Number) or positive/negative infinity.

NaN specifically causes two kinds of problems:

  1. The result of a buggy calculation might be NaN, and instead of causing a problem right then and there, the NaN value might be carried forward, infecting the results of other calculations, until leading to some problem way down the line. This can be tedious to debug.
  2. The properties of NaN can make working with floating-point numbers tedious. In Rust, floating-point types (and usually any types containing them) don't implement Eq or Hash, making it impossible to put them into HashSet, or as keys into HashMap. The same goes for similar sets/maps.

Especially that second point is turning into a problem. It can be worked around, but it makes things more difficult than they should be. I've used Decorum for such workarounds before, but that's not ideal. It works, for example to convert a Point<3> into a [R64; 3], to put into a HashSet. But the necessity of such workarounds always puts up hurdles in front of the correct and obvious solution. It's never a big deal, but in aggregate, it's starting to weight us down.

I think it's time to address this problem once and for all, by switching to math types that implement Eq/Hash, and use those everywhere. Here's my rough plan:

  • Add a new type to the math module to represent scalar values (could be called Scalar). It would wrap an f64 and provide easy conversion from/to f64.
  • When converting from f64, Scalar would check whether the f64 value is all proper and regular, and if not, panic right away. This would expose any bugs right where they happen.
  • math::Point and math::Vector would become structs that wrap an array of Scalar and provide easy conversion from/to its nalgebra equivalents. Like scalar, the "from" conversions would be panicky, to expose bugs right away.
  • Many methods could accept arguments like impl Into<math::Point<3>>. That would still allow us to do a quick point![1., 2. 3.] in test code.

Initially, that could be all there is. The easy conversions would always provide access to any nalgebra operations, and enable a piecemeal migration. Over time, we could add more conveniences, like implementing the std::ops traits directly. These implementations would defer to nalgebra, of course.

One concern here is how performance would be affected, as this plan would add panicky checks to lots of places. I suspect that we can optimize performance-sensitive code by working only with nalgebra/f64, and only doing the conversion at the end. But I do think the added robustness and ease of development is worth some performance cost.

Here are some alternatives to the above plan that I've dismissed:

  1. Keep using the nalgebra types directly, instead of adding custom Point/Vector types. This is possible, but would cost us the easy From/Into conversions we could otherwise have.
  2. Use nalgebra with Decorum's R64, instead of adding a custom Scalar type. In addition to having the same disadvantage as the previous item, I've tried this in the past, and ran into problems. R64 doesn't implement certain nalgebra traits that are required for some operations (I don't recall details). This could be overcome, but would require upstream work.

I'm thinking of working on this soon, but I'm not sure. On the one hand, I was hoping not to get distracted from #97, but on the other hand, those workaround I need to make are distracting too. Very tempting to just get this over with now.

@hannobraun hannobraun added type: development Work to ease development or maintenance, without direct effect on features or bugs topic: core Issues relating to core geometry, operations, algorithms labels Feb 16, 2022
@hannobraun
Copy link
Owner Author

I've started looking into this. So far, I'm just experimenting, and not yet committed to seeing it through.

@WeirdConstructor
Copy link

Another more or less minor thing with floating point numbers are denormalized floats. These are commonly very very small floats that are represented differently, and they can cause a slow down in processing. This is of course more an issue for the DSP code, as the performance margins there are much much more constrained by the real time aspect of computing sound samples.

@hannobraun
Copy link
Owner Author

Interesting, thanks for the insight, @WeirdConstructor! I don't think I've come across this before (or I have, and forgot all about it, which is also quite possible). I wonder, if this will become relevant for Fornjot too, and what could be done about it.

@hannobraun
Copy link
Owner Author

Having read up about it a bit, those kinds of numbers could could probably be detected when constructing the hypothetical Scalar value, and the value could then be set to zero. Not sure, if this is going to be necessary, but good to know.

@hannobraun
Copy link
Owner Author

I've been working on this for the last few days. I think I just got done with all the preparation, and am now ready to realize the actual benefits promised here. I also ran out of time for today though, so that will have to come later.

I don't feel comfortable with all the rote code that I had to add to get this far, but I guess that's just what's required. At least the ugliness is mostly confined to the math module. The whole point is that the rest of the code can soon be more robust, and no longer require ugly workarounds whenever Eq and Hash are needed.

Long-term, it's probably better to try and improve the upstream crates, namely nalgebra and Decorum, so we can get rid of this stuff again. I don't think I'll have time to do this in the foreseeable future. If anyone reading this is interested in this, you should look into that! It's a change that the whole ecosystem could benefit from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Issues relating to core geometry, operations, algorithms type: development Work to ease development or maintenance, without direct effect on features or bugs
Projects
None yet
2 participants