-
Notifications
You must be signed in to change notification settings - Fork 35
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 support for measurement type #884
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we need to update sample-constants first?
9242bd8
to
195e8ac
Compare
support measurement type
195e8ac
to
55b71b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it gets more and more complicated, I think we should have a unit test for it. Use a small fake schema with different thingtalk types to test it. Probably a good idea to have a parameter to control how many examples per annotation - so we can set it to 1 to make the unit test shorter.
if (argument.type instanceof Type.Measure) | ||
argumentCanonical = '#' + argumentCanonical; | ||
else | ||
argumentCanonical = argumentCanonical + ' #'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this?
tool/sample-synthetic-data.ts
Outdated
function retrieveSampleValues(classDef : Ast.ClassDef, | ||
sampleMeta : Record<string, Constant[]>, | ||
fname : string, | ||
arg : Ast.ArgumentDef) : string[] { | ||
if (arg.type instanceof Type.Enum) | ||
return arg.type.entries!.slice(0, 10).map(utils.clean); | ||
if (arg.type instanceof Type.Measure) | ||
return generateRandomIntArray(100).map(String); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include number, time, date, and location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
Added support for the measurement type