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

Chores #445

Merged
merged 38 commits into from
Nov 7, 2023
Merged

Chores #445

merged 38 commits into from
Nov 7, 2023

Conversation

sorki
Copy link
Contributor

@sorki sorki commented Oct 26, 2023

  • Fixes bunch of warnings produced by more recent GHC
  • CI updates
  • Bunch of random small fixes and improvements I'm discovering while debugging QuickCheck failures from QuickCheck test failures #441 which are caused by the improved Arbitrary Double generator that now also generates exact zeroes.

```
Graphics/Implicit/Definitions.hs:113:2: warning: [-Wtype-equality-requires-operators]
    The use of ‘~’ without TypeOperators
    will become an error in a future GHC release.
    Suggested fix: Perhaps you intended to use TypeOperators
    |
113 | $(makeWrapped ''ℝ3')
```
@sorki sorki force-pushed the srk/chores branch 2 times, most recently from 05dbfa5 to 5aa2c43 Compare October 31, 2023 14:18
There are some new warnings about specialisation of imported functions
like
```
programs/docgen.hs: error: [-Wall-missed-specialisations, -Werror=all-missed-specialisations]
    Could not specialise imported function ‘ghc-prim:GHC.Classes.$fEq[]_$c==’
    Probable fix: add INLINABLE pragma on ‘ghc-prim:GHC.Classes.$fEq[]_$c==’
```
which we can't do much about. Don't fail in CI on these.
This fixes show for rotate3
so cheaper `r /= 0` is evaluated first and it can sometimes fail faster
without evaluating more expensive `abs (x-y) < r` operation.
Before, only `Insidedness` was rendered, now we also show some more
details

```
  3) symbolic obj 3, inverses, scale inverse
       Falsified (after 1 test and 1 shrink):
         V3 0.0 0.0 3.328273472768273e-2
         (sphere 0.0,(V3 0.0 0.0 0.0,()))

         TestResult:
           | Outside
           | scale (V3 NaN NaN 1.0) (sphere 0.0)
           | Sampled at V3 0.0 0.0 0.0 returns NaN
          /=
         TestResult:
           | Surface
           | sphere 0.0
           | Sampled at V3 0.0 0.0 0.0 returns 0.0
```
so `NaN` doesn't result in `Outside`. We also make sure that two
NaNFails are never equal, so the property producing NaN always fails.
this (introduced by me) looks like a good idea (and identity) but it isn't
since sometimes we want to reset rounding to zero for nested objects.

This broke `prop "withRounding always takes the last value idempotent"`.
Graphics/Implicit/Definitions.hs Show resolved Hide resolved
Graphics/Implicit/ObjectUtil/GetImplicit2.hs Outdated Show resolved Hide resolved
@@ -56,7 +56,7 @@ getImplicitShared ctx (UnionR _ []) =
getImplicitShared ctx (UnionR r symbObjs) = \p ->
rminimum r $ fmap (flip (getImplicit' ctx) p) symbObjs
getImplicitShared ctx (IntersectR _ []) =
getImplicitShared @obj ctx Full
getImplicitShared @obj ctx Empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this just wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, based on https://en.wikipedia.org/wiki/Intersection_(set_theory)#Algebraic_properties

The intersection of any set with the empty set results in the empty set;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also our implicit space (or starting object?) seems to be emptySpace - if we have union [oneObject] or just oneObject (without implicit union xD) it seems like it can only be unioned with emptySpace, because if it was a fullSpace we would get fullSpace instead of oneObject.

programs/extopenscad.hs Outdated Show resolved Hide resolved

------------------------------------------------------------------------------
-- | The number of decimal points we need to agree to assume two 'Double's are
-- equal.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excuse me while i hurl. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the reason it wasn't required was that Arbitrary Double / Gen Double wasn't generating small enough or close to zero things and transforms. Before, there was epsilon = 5 and currently even epsilon = 16 works quite well but eventually, with large enough --qc-max-success it breaks. This seems to hold up to --qc-max-success=10M

Related to Haskell-Things#441

This will get factored out into canonicalization pass
It doens't hold for cases where one scaling has zero component.

Adjust QC tests to generate scaling vectors without zero components.
@sorki
Copy link
Contributor Author

sorki commented Nov 1, 2023

I still want to factor out guards into canonicalization function, that will also solve it for getBox2|3.

@sorki sorki force-pushed the srk/chores branch 5 times, most recently from e4c04eb to 7c647c6 Compare November 7, 2023 17:21
@sorki sorki marked this pull request as ready for review November 7, 2023 17:29
Implements recursive tree rewriting to handle
identities, transformation merging, sanitization
and basic handling of degenerate objects.
It didn't remove temporary files on successful runs, filling up tmp.

Make `goldenFormat'` higher order so it can be used by both 2D and 3D.

Render pretty fail message.
-- Copyright 2015 2016, Mike MacHenry ([email protected])
-- Released under the GNU AGPLV3+, see LICENSE

-- | This module implements canonicalization pass that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- | This module implements canonicalization pass that
-- | This module implements a canonicalization pass that

@@ -112,7 +112,7 @@ buildShared (WithRounding r obj) | r == 0 = build obj

buildShared(UnionR _ _) = error "cannot provide roundness when exporting openscad; unsupported in target format."
buildShared(IntersectR _ _) = error "cannot provide roundness when exporting openscad; unsupported in target format."
buildShared(DifferenceR _ _ _) = error "cannot provide roundness when exporting openscad; unsupported in target format."
buildShared(DifferenceR {}) = error "cannot provide roundness when exporting openscad; unsupported in target format."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why quash the argument count just here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hlint complains about it when there are 3 and more

@@ -3,10 +3,6 @@
-- Copyright (C) 2016, Julia Longtin ([email protected])
-- Released under the GNU AGPLV3+, see LICENSE

-- allow us to specify what package to import what module from.
-- We don't actually care, but when we compile our haskell examples, we do.
{-# LANGUAGE PackageImports #-}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what made this redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea! (found by hlint).

@julialongtin julialongtin merged commit 2d026d3 into Haskell-Things:master Nov 7, 2023
4 checks passed
sorki added a commit to sorki/ImplicitCAD that referenced this pull request Nov 12, 2023
@sorki sorki mentioned this pull request Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants