promote ℝ to an instance of a typeclass#225
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase to treat ℝ as a dedicated numeric type backed by a new ℚ typeclass, and migrates call sites to use math operations/constants (π, sqrt, cbrt, normalizeℝ2/3, etc.) provided via Graphics.Implicit.Definitions rather than Prelude/vector-space helpers.
Changes:
- Introduces
Graphics.Implicit.RationalUtil(definesℚand theℝnewtype) andGraphics.Implicit.RationalFunctions(rational math helpers). - Updates modeling, export, and OpenSCAD interpreter code to import numeric operations/constants from
Graphics.Implicit.Definitionsand replacesnormalized/magnitudepatterns withnormalizeℝ*/magnitudeSq+sqrt. - Updates build configuration to include the new modules and tweaks profiling/dynamic build flags.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| programs/implicitsnap.hs | Switches cube-root usage to cbrt and moves math imports to Definitions. |
| programs/extopenscad.hs | Same cbrt/math import refactor in CLI executable. |
| programs/Benchmark.hs | Uses π/cos from Definitions and adds numeric defaulting. |
| implicit.cabal | Adds new modules to the library; adjusts executable GHC options (dynamic commented). |
| Makefile | Updates commented profiling flag to library profiling. |
| Graphics/Implicit/RationalUtil.hs | Adds ℚ typeclass and ℝ newtype + instances (core of the change). |
| Graphics/Implicit/RationalFunctions.hs | Adds rational trig/sqrt/cbrt/normalize helpers (future precision support). |
| Graphics/Implicit/ObjectUtil/GetImplicit3.hs | Replaces normalized, pi, and (** (1/3)) usage with normalizeℝ3, π, cbrt. |
| Graphics/Implicit/ObjectUtil/GetImplicit2.hs | Moves sqrt/sin/cos imports to Definitions. |
| Graphics/Implicit/ObjectUtil/GetBox3.hs | Uses infty/neginfty from Definitions instead of local 1/0. |
| Graphics/Implicit/ObjectUtil/GetBox2.hs | Uses infty/neginfty and replaces magnitude with sqrt . magnitudeSq. |
| Graphics/Implicit/MathUtil.hs | Replaces distance/normalized with distanceSq + sqrt + normalizeℝ2. |
| Graphics/Implicit/IntegralUtil.hs | Trivial whitespace-only change. |
| Graphics/Implicit/ExtOpenScad/Util/OVal.hs | Routes numeric-to-ℕ conversion via fromℝtoℕ and enables FlexibleInstances. |
| Graphics/Implicit/ExtOpenScad/Primitives.hs | Migrates trig/π usage to Definitions. |
| Graphics/Implicit/ExtOpenScad/Parser/Util.hs | Uses powℝ for exponent parsing instead of (**). |
| Graphics/Implicit/ExtOpenScad/Parser/Expr.hs | Import list cleanup for refactor. |
| Graphics/Implicit/ExtOpenScad/Default.hs | Moves constants/functions (π, trig, powℝℝ, etc.) to Definitions. |
| Graphics/Implicit/Export/Util.hs | Uses normalizeℝ3 and replaces realToFrac-based centroid scaling with ℕ/ℝ conversions. |
| Graphics/Implicit/Export/TriangleMeshFormats.hs | Uses normalizeℝ3 for normals. |
| Graphics/Implicit/Export/TextBuilderUtils.hs | Fixes float formatting by converting via fromℝtoFloat. |
| Graphics/Implicit/Export/SymbolicObj2.hs | Uses π, sqrt, magnitudeSq instead of pi/magnitude. |
| Graphics/Implicit/Export/SymbolicFormats.hs | Uses π from Definitions and adjusts rad/deg conversion. |
| Graphics/Implicit/Export/Symbolic/Rebound3.hs | Adds default (ℝ) to avoid ambiguous numeric literals with new ℝ. |
| Graphics/Implicit/Export/Symbolic/Rebound2.hs | Adds default (ℝ) similarly. |
| Graphics/Implicit/Export/Render/TesselateLoops.hs | Switches normalization/magnitude to normalizeℝ3 and sqrt . magnitudeSq. |
| Graphics/Implicit/Export/Render/RefineSegs.hs | Uses normalizeℝ2 and sqrt . magnitudeSq for geometry/refinement logic. |
| Graphics/Implicit/Export/Render/GetSegs.hs | Moves sqrt import to Definitions. |
| Graphics/Implicit/Export/RayTrace.hs | Uses normalizeℝ3 and sqrt . magnitudeSq for ray math. |
| Graphics/Implicit/Definitions.hs | Re-exports the new RationalUtil API via module R and removes the old type ℝ = Double model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- Copyright 2016, Julia Longtin (julial@turinglace.com) | ||
| -- Released under the GNU AGPLV3+, see LICENSE | ||
|
|
||
| -- for the type arithmatic in the types of normalizedℝ(2,3). |
There was a problem hiding this comment.
Typo in comment: “arithmatic” → “arithmetic”.
| -- for the type arithmatic in the types of normalizedℝ(2,3). | |
| -- for the type arithmetic in the types of normalizedℝ(2,3). |
|
|
||
| import GHC.Real (Ratio((:%))) | ||
|
|
||
| -- FIXME: since the taylor series for asin is acurate only in a certain range, rollover in that range. |
There was a problem hiding this comment.
Typo in comment: “acurate” → “accurate”.
| -- FIXME: since the taylor series for asin is acurate only in a certain range, rollover in that range. | |
| -- FIXME: since the taylor series for asin is accurate only in a certain range, rollover in that range. |
| (*^) (ℝ a) (ℝ b) = ℝ $ (P.*) a b | ||
| {-# INLINABLE (*^) #-} | ||
|
|
||
| instance InnerSpace ℝ where | ||
| (<.>) = (P.*) |
There was a problem hiding this comment.
Data.VectorSpace.InnerSpace requires (<.>) :: ℝ -> ℝ -> Scalar ℝ (i.e., ℝ), but this implementation uses P.* (Double multiplication), which won’t typecheck with the ℝ newtype. Use (*) on ℝ (or pattern-match and rewrap) instead of P.* here.
| (*^) (ℝ a) (ℝ b) = ℝ $ (P.*) a b | |
| {-# INLINABLE (*^) #-} | |
| instance InnerSpace ℝ where | |
| (<.>) = (P.*) | |
| (*^) = (*) | |
| {-# INLINABLE (*^) #-} | |
| instance InnerSpace ℝ where | |
| (<.>) = (*) |
| (.-.) = (P.-) | ||
| {-# INLINABLE (.-.) #-} | ||
| (.+^) = (P.+) |
There was a problem hiding this comment.
AffineSpace operations here are defined using P.+/P.- (Double operations), which won’t typecheck for the ℝ newtype and will fail the build under -Werror. Define (.-.)/(.+^) using the ℝ numeric instances (or unwrap/rewrap explicitly) instead.
| (.-.) = (P.-) | |
| {-# INLINABLE (.-.) #-} | |
| (.+^) = (P.+) | |
| (ℝ a) .-. (ℝ b) = ℝ $ (P.-) a b | |
| {-# INLINABLE (.-.) #-} | |
| (ℝ a) .+^ (ℝ b) = ℝ $ (P.+) a b |
|
|
||
| module Graphics.Implicit.RationalUtil (ℚ(..), ℝ) where | ||
|
|
||
| import Prelude (RealFrac(properFraction, ceiling, floor, round, truncate), Fractional(fromRational, (/)), Ord, Double, Show(show), Eq, Num((+), (*), abs, negate, signum, fromInteger), Real(toRational), Read(readsPrec), ($), seq, (==), floor, Float, map) |
There was a problem hiding this comment.
The explicit Prelude import list includes floor twice (once via RealFrac(..., floor, ...) and again as a standalone import). With this repo’s -Wall -Werror, this will be promoted to an error (-Wduplicate-imports). Remove the duplicate import item.
| import Prelude (RealFrac(properFraction, ceiling, floor, round, truncate), Fractional(fromRational, (/)), Ord, Double, Show(show), Eq, Num((+), (*), abs, negate, signum, fromInteger), Real(toRational), Read(readsPrec), ($), seq, (==), floor, Float, map) | |
| import Prelude (RealFrac(properFraction, ceiling, floor, round, truncate), Fractional(fromRational, (/)), Ord, Double, Show(show), Eq, Num((+), (*), abs, negate, signum, fromInteger), Real(toRational), Read(readsPrec), ($), seq, (==), Float, map) |
| {-# INLINABLE log #-} | ||
| sqrt (ℝ x) = ℝ $ P.sqrt x | ||
| {-# INLINABLE sqrt #-} | ||
| cbrt (ℝ x) = ℝ $ (P.**(1/3)) x |
There was a problem hiding this comment.
cbrt implemented as x ** (1/3) returns NaN for negative inputs with Doubles (e.g. (-8) ** (1/3)). Since cbrt is now part of the public numeric API, it should be sign-preserving (e.g. signum x * abs x ** (1/3)) to behave as a real cube root.
| cbrt (ℝ x) = ℝ $ (P.**(1/3)) x | |
| cbrt (ℝ x) = ℝ $ P.signum x P.* (P.abs x P.** (1 P./ 3)) |
| import Data.VectorSpace (Scalar, InnerSpace, (<.>)) | ||
|
|
||
| import Graphics.Implicit.FastIntUtil as F (Fastℕ(Fastℕ), fromFastℕ, toFastℕ) | ||
| import Graphics.Implicit.RationalUtil as R (ℚ(minℝ, π, sqrt, cbrt, powℝ, powℝℝ, exp, log, cos, sin, tan, asin, acos, atan, sinh, cosh, tanh, atan2, fromℝ, toℝ, normalizeℝ2, normalizeℝ3, powℝ, (%), infty, neginfty), ℝ, fromFastℕtoℝ, fromℕtoℝ, fromℝtoℕ, fromℝtoFloat) |
There was a problem hiding this comment.
This import list includes powℝ twice. With -Wall -Werror enabled in this repo, the duplicate import item will fail the build (-Wduplicate-imports). Remove the duplicated powℝ entry.
| import Graphics.Implicit.RationalUtil as R (ℚ(minℝ, π, sqrt, cbrt, powℝ, powℝℝ, exp, log, cos, sin, tan, asin, acos, atan, sinh, cosh, tanh, atan2, fromℝ, toℝ, normalizeℝ2, normalizeℝ3, powℝ, (%), infty, neginfty), ℝ, fromFastℕtoℝ, fromℕtoℝ, fromℝtoℕ, fromℝtoFloat) | |
| import Graphics.Implicit.RationalUtil as R (ℚ(minℝ, π, sqrt, cbrt, powℝ, powℝℝ, exp, log, cos, sin, tan, asin, acos, atan, sinh, cosh, tanh, atan2, fromℝ, toℝ, normalizeℝ2, normalizeℝ3, (%), infty, neginfty), ℝ, fromFastℕtoℝ, fromℕtoℝ, fromℝtoℕ, fromℝtoFloat) |
No description provided.