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

Add multiply-by-positive-constant primitive hmulpos. #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cheater
Copy link
Contributor

@cheater cheater commented Aug 13, 2017

Following your tips I have rewritten the "concatenation" function a little and it seems I have found a simple primitive that should provide stronger monotonicity.

varsAsVect xs = vOut
  -- | Take a list of scalar variables and output them as a single vector Expr
  where
    vOut = foldl' (+~) (head vs) (tail vs)
    xsIndices = zip ([0..] :: [Int]) xs
    vs = f <$> xsIndices
    f (ind, x) = v *~+ x
      where
        v = EConst $ orthonormal ind

    orthonormal i = assoc (n, 1) 0.0 [((i, 0), 1.0)]
    n = length xs

here *~+ is exactly the same as *~ except you can only have non-negative constants on the left, or it will throw an exception. Is my understanding correct that this requirement on the left argument will preserve monotonicity of the right argument?

@chrisnc
Copy link
Owner

chrisnc commented Aug 23, 2017

I'm still skeptical about adding more exports. Also it seems unrelated to the other changes you're making.

infixl 7 *~+
(*~+) :: (ApplyVex 'Affine 'Nondec v1 m1 ~ v2, ValidVex v2)
=> Expr 'Affine 'Const -> Expr v1 m1 -> Expr v2 (ApplyMon 'Nondec m1)
(*~+) (EConst a) e
Copy link
Owner

Choose a reason for hiding this comment

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

Can you collapse this to just (*~+) = hmulpos? While you're here might as well to the same for (*~). I did that for the other operator aliases but I guess I missed one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I just did it that way because you had (*~) and hmul.

=> Expr 'Affine 'Const -> Expr v1 m1 -> Expr v2 (ApplyMon 'Nondec m1)
hmulpos (EConst a) e
| 0 <= minElement a = apply (MulPos a) e
| otherwise = error "The left argument of a left-positive multiply must have non-negative entries"
Copy link
Owner

Choose a reason for hiding this comment

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

"left-positive" seems redundant. You already mentioned you're referring to the left argument. Same for the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so the issue i'm trying to solve here is that when a non-positive entry is entered, we see an error like this:

> solve 1
([("x1",(1><1)
 [ 10.0 ]),("x2",(1><1)
 [ 200.0 ]),("x3",(1><1)
 [ 400.0 ]),("lineVar",(1><1)
 [ 10.0 ])],*** Exception: The left argument of a left-positive multiply must have non-negative entries
CallStack (from HasCallStack):
  error, called at src/HVX/Primitives.hs:78:17 in hvx-0.3.1.0-HnMwpnJtEwx2GYI4IYHzYJ:HVX.Primitives

so we need to identify the function throwing the error, otherwise the user won't know why the error is happening and will be unable to debug it. We also identify the reason the function is throwing the error, so that the user doesn't have to look at the hmulpos source code to figure that out.

How would you do both without being redundant but maintaining a clear description of what went wrong and where?

Copy link
Owner

@chrisnc chrisnc Aug 23, 2017

Choose a reason for hiding this comment

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

Sorry, I just meant that you could change left-positive to positive. This still distinguishes it from the generic multiply-by-constant function.

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 see! I'm worried "positive multiply" might imply that the right argument has to be positive too.

Copy link
Owner

Choose a reason for hiding this comment

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

But the message as a whole doesn't imply that. I think it's fine.

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'm confused now :) ok can we just leave it as is? I think no one will find it offensive and we'll have ticked off this pr :)

Copy link
Owner

Choose a reason for hiding this comment

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

Still change it from "left-positive" to "positive". After that I think it's unambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, i just pushed

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