feat: compile non-property decls to loss - #1132
Conversation
MatthewDaggitt
changed the title
MatthewDaggitt
left a comment
MatthewDaggitt
left a comment
There was a problem hiding this comment.
Looks like the right idea, thank you! Got a few minor suggestions for changes.
|
|
||
| ### Loss backend | ||
|
|
||
| * Declaration flag can now be used to specify non-property declarations that will also be compiled with the chosed DL to a callable function. |
There was a problem hiding this comment.
- The
--declarationflag. - "chosed" -> "chosen"
- "to a callable function"? Is this true if the declaration is
x : Int; x = 2? That shouldn't be compiled to a callable right?
| | otherwise -> return Nothing | ||
| DefFunction p ident ann typ expr | ||
| | isPropertyDecl decl -> Just <$> convertPropertyDecl p ident ann typ expr | ||
| | isPropertyDecl decl || Set.member (nameOf ident) requestedDecls -> |
There was a problem hiding this comment.
Hmm should we be changing the semantics so that it only compiles the requested declarations? This seems a little odd to compile both, even if the user has only requested specific, non-property declarations?
There was a problem hiding this comment.
I was thinking more about the context of needing more from a spec, not just the properties, but your approach makes more sense in general
| compileToLossFunction LossOptions {..} typedProg outputAsJSON = | ||
| logCompilerPass Loss $ do | ||
| lossTensorProg <- convertToLossTensors differentiableLogicID typedProg | ||
| lossTensorProg <- convertToLossTensors differentiableLogicID (Set.fromList declarationsToCompile) typedProg |
There was a problem hiding this comment.
Can we put the declaration set calculation on a separate line?
| @@ -0,0 +1,2 @@ | |||
| Error in file 'spec.vcl' at Line 4, Columns 1-9: compiling declaration 'someList' as a loss output (supported leaf types: `Bool`, `Real`, `Tensor Bool _`, `Tensor Real _`, `Vector _ _`) is not currently implemented | |||
There was a problem hiding this comment.
This limitation feels a bit artificial. It should be possible to get it working for any declaration...
| convertOutputDecl p ident ann typ value = do | ||
| leafType <- stripPiTypes typ | ||
| case toTypeValue leafType of | ||
| VBoolTensorType {} -> emit leafType |
There was a problem hiding this comment.
This code is completely baffling. Why are we doing the same thing for every type value? We should be doing something different for each one...
| return $ DefFunction p ident ann lossType lossExpr | ||
|
|
||
| stripPiTypes :: (MonadLogic m) => VType Builtin -> m (VType Builtin) | ||
| stripPiTypes typ = case toTypeValue typ of |
There was a problem hiding this comment.
This doesn't look right either. Stripping all Pi types off means we can't convert functions... Is this trying to get rid of the implicit functions? If so I don't think they should exist at this point because the whole spec has been monomorphised?
Is there a concrete test case that we need to do this for?
| VVectorType tElem _d -> convertVectorProperty tElem | ||
| _ -> unexpectedExprError currentPass "Impossible property type" | ||
| convertMultiOutput :: (MonadLogic m) => VType Builtin -> Value Builtin -> m (Value LossBuiltin) | ||
| convertMultiOutput typ = case toTypeValue typ of |
There was a problem hiding this comment.
Can we rename this simply convertTypedValue?
|
|
||
| convertVectorProperty :: (MonadLogic m) => VType Builtin -> Value Builtin -> m (Value LossBuiltin) | ||
| convertVectorProperty typ value = do | ||
| convertVectorOutput :: (MonadLogic m) => VType Builtin -> Value Builtin -> m (Value LossBuiltin) |
There was a problem hiding this comment.
Can we rename this simply convertVectorValue? (and similar renamings for the other functions).
# Conflicts: # ChangeLog.md # vehicle/src/Vehicle/Backend/Loss.hs # vehicle/src/Vehicle/Backend/Loss/LossCompilation.hs # vehicle/src/Vehicle/Compile.hs
2 participants
Add ability to compile non-property declarations to callable functions translated with the chosen DL. This is done by passing the name of the declaration via the
--declarationflag, same as for properties.