-
Notifications
You must be signed in to change notification settings - Fork 0
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
Process positional arguments #134
base: main
Are you sure you want to change the base?
Changes from 6 commits
df69fd5
3b881a6
2899de3
731b664
a4cf2cd
d6b16ed
aeca489
61f1b1b
e8b508c
109e0e4
b7d9d5c
6dbbadd
e5c5b80
1046cff
8c489aa
130f807
55cadea
c83eccf
4361e2e
53c44f1
23ae949
94a005f
528a3a7
a3cfae6
92cf918
b145a53
72d53c6
d05963b
fffd0d0
49065c9
f53fe07
e52d371
ca0c4d2
914c417
ebd3880
aa4f6cd
19a8a72
f92872c
1eea97a
f7cd804
a107678
a835e8b
594ea9d
dbf19ad
c182a37
d3239b4
9a60cbf
1cb7502
f710611
dacd17a
5673690
474acdf
ccce776
4e16587
5d72476
b4358fd
54ba275
afd01ff
1ca2997
afdbbb0
4161ad7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,7 +143,7 @@ public HybridizationParameters(IProgressMonitor monitor) throws BadLocationExcep | |
decoratorsType tfFunctionDecorator = null; | ||
|
||
// Declaring definitions of the decorator, if it contains multiple definitions there might be more than one in this set. Since | ||
// we are dealing with tf.function, we expect only one. | ||
// we are dealing with tf.function, we check we it has one definition. | ||
Definition declaringDefinition = null; | ||
|
||
// Iterate through the decorators of the function | ||
|
@@ -164,61 +164,62 @@ public HybridizationParameters(IProgressMonitor monitor) throws BadLocationExcep | |
if (potentialDeclaringDefitinion.iterator().hasNext()) | ||
declaringDefinition = potentialDeclaringDefitinion.iterator().next(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is there not an else case? I'm not seeing the logic here. Can be greatly helped with some comments in the code of the case analysis. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added comments for this conditional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what variable would be null and why. Rewrite the logic to handle the else case here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have rewritten so we handle the else case here |
||
else | ||
throw new IllegalStateException("Can't determine tf.function decorator defintion."); | ||
throw new IllegalStateException(String.format( | ||
"Can't find declaring definition for selection: %s in line: %s, file: %s, and project: %s.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error message is vague and doesn't resemble the one that was modified. |
||
selection.getSelectedText(), selection.getLineWithoutCommentsOrLiterals().strip(), | ||
Function.this.containingFile.getName(), Function.this.nature.getProject())); | ||
} | ||
} catch (AmbiguousDeclaringModuleException e) { | ||
throw new IllegalStateException("Can't determine whether decorator: " + decorator + " is hybrid.", e); | ||
} | ||
} // We expect to have the last tf.function decorator in tfFunctionDecorator | ||
|
||
// Getting tf.functions Python definition arguments. | ||
ArrayList<String> argumentIdDeclaringDefintion = getTfFunctionPythonDefinitionArguments(declaringDefinition); | ||
ArrayList<String> argumentIdDeclaringDefintion = getPythonDefinitionArguments(declaringDefinition); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is a "Python definition argument?" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting the argument from the Python definition |
||
|
||
if (tfFunctionDecorator != null) | ||
// tfFunctionDecorator must be an instance of Call, because that's the only way we have parameters. | ||
if (tfFunctionDecorator.func instanceof Call) { | ||
Call callFunction = (Call) tfFunctionDecorator.func; | ||
|
||
exprType[] tfFunctionPositionalArgs = callFunction.args; | ||
|
||
// We iterate over the tf.function's parameters positions. | ||
for (int i = 0; i < callFunction.args.length; i++) { | ||
// From the position i, we use the tf.function's definition to verify which parameter we are analyzing. | ||
String evaluatedArgument = argumentIdDeclaringDefintion.get(i); | ||
|
||
// Matching the arguments from the definition and the arguments from the code being analyzed. | ||
if (evaluatedArgument.equals(FUNC)) | ||
// Found parameter func | ||
this.funcParamExists = true; | ||
else if (evaluatedArgument.equals(INPUT_SIGNATURE)) | ||
// Found parameter input_signature | ||
this.inputSignatureParamExists = true; | ||
else if (evaluatedArgument.equals(AUTOGRAPH)) | ||
// Found parameter autograph | ||
this.autoGraphParamExists = true; | ||
// In our accepted interval version ([2.0,2.11]) of the API allows parameter names jit_compile and | ||
// deprecated name experimental_compile. | ||
else if (evaluatedArgument.equals(JIT_COMPILE) || evaluatedArgument.equals(EXPERIMENTAL_COMPILE)) | ||
// Found parameter jit_compile/experimental_compile | ||
this.jitCompileParamExists = true; | ||
// In our accepted interval version ([2.0,2.11]) of the API allows parameter names reduce_retracing | ||
// and deprecated name experimental_relax_shapes. | ||
else if (evaluatedArgument.equals(REDUCE_RETRACING)) | ||
// Found parameter reduce_retracing | ||
this.reduceRetracingParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_RELAX_SHAPES)) | ||
// Found parameter experimental_relax_shapes | ||
this.reduceRetracingParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_IMPLEMENTS)) | ||
// Found parameter experimental_implements | ||
this.experimentalImplementsParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_AUTOGRAPH_OPTIONS)) | ||
// Found parameter experimental_autograph_options | ||
this.experimentalAutographOptionsParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_FOLLOW_TYPE_HINTS)) | ||
// Found parameter experimental_follow_type_hints | ||
this.experimentaFollowTypeHintsParamExists = true; | ||
else | ||
throw new IllegalArgumentException("Unable to process tf.function argument."); | ||
} | ||
if (tfFunctionPositionalArgs.length <= argumentIdDeclaringDefintion.size()) { | ||
for (int i = 0; i < tfFunctionPositionalArgs.length; i++) { | ||
// From the position i, we use the tf.function's definition to verify which parameter we are analyzing. | ||
String evaluatedArgument = argumentIdDeclaringDefintion.get(i); | ||
|
||
// Matching the arguments from the definition and the arguments from the code being analyzed. | ||
if (evaluatedArgument.equals(FUNC)) | ||
this.funcParamExists = true; | ||
else if (evaluatedArgument.equals(INPUT_SIGNATURE)) | ||
this.inputSignatureParamExists = true; | ||
else if (evaluatedArgument.equals(AUTOGRAPH)) | ||
this.autoGraphParamExists = true; | ||
// In our accepted interval version ([2.0,2.11]) of the API allows parameter names jit_compile and | ||
// deprecated name experimental_compile. | ||
else if (evaluatedArgument.equals(JIT_COMPILE) || evaluatedArgument.equals(EXPERIMENTAL_COMPILE)) | ||
this.jitCompileParamExists = true; | ||
// In our accepted interval version ([2.0,2.11]) of the API allows parameter names reduce_retracing | ||
// and deprecated name experimental_relax_shapes. | ||
else if (evaluatedArgument.equals(REDUCE_RETRACING)) | ||
this.reduceRetracingParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_RELAX_SHAPES)) | ||
this.reduceRetracingParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_IMPLEMENTS)) | ||
this.experimentalImplementsParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_AUTOGRAPH_OPTIONS)) | ||
this.experimentalAutographOptionsParamExists = true; | ||
else if (evaluatedArgument.equals(EXPERIMENTAL_FOLLOW_TYPE_HINTS)) | ||
this.experimentaFollowTypeHintsParamExists = true; | ||
else | ||
throw new IllegalArgumentException("Unable to process tf.function argument at position " + i); | ||
} | ||
} else | ||
throw new IllegalArgumentException( | ||
"Unable to process tf.function argument. The number of arguments " + tfFunctionPositionalArgs.length | ||
+ " exceeds the accepted number of argumets " + argumentIdDeclaringDefintion.size()); | ||
tatianacv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Processing keywords arguments | ||
// If we have keyword parameter, afterwards, we cannot have positional parameters because it would result in invalid | ||
|
@@ -256,20 +257,18 @@ else if (name.id.equals(EXPERIMENTAL_AUTOGRAPH_OPTIONS)) | |
else if (name.id.equals(EXPERIMENTAL_FOLLOW_TYPE_HINTS)) | ||
// Found parameter experimental_follow_type_hints | ||
this.experimentaFollowTypeHintsParamExists = true; | ||
else | ||
throw new IllegalArgumentException("Unable to process tf.function argument."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this removed? Now, we are missing the else case here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because this is for keyword arguments |
||
} | ||
} | ||
} // else, tf.function is used without parameters. | ||
} | ||
|
||
/** | ||
* Get the tf.function parameter names from the {@link Definition}. | ||
* Get the parameter names from the {@link Definition}. | ||
* | ||
* @param declaringDefinition The Definition to use. | ||
* @return An array with the names of the arguments given by {@link Definition}. | ||
*/ | ||
private ArrayList<String> getTfFunctionPythonDefinitionArguments(Definition declaringDefinition) { | ||
private ArrayList<String> getPythonDefinitionArguments(Definition declaringDefinition) { | ||
// Python source arguments from the declaring definition | ||
exprType[] declaringArguments = null; | ||
|
||
|
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.
It's not only the comment. The checking must be in the code.
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.
There is a check in the code.