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

anomaly localization integration step 3 #114

Merged
merged 3 commits into from
Jan 19, 2022
Merged

anomaly localization integration step 3 #114

merged 3 commits into from
Jan 19, 2022

Conversation

wnbts
Copy link
Contributor

@wnbts wnbts commented Dec 30, 2021

Signed-off-by: lai 57818076+wnbts@users.noreply.github.com

Description

the changes are for step 3 in anomaly localization integration

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@wnbts wnbts requested a review from a team December 30, 2021 23:30
Signed-off-by: lai <57818076+wnbts@users.noreply.github.com>
/**
* Output of localized results.
*/
@Data
public class Output implements ToXContent {
@NoArgsConstructor
public class Output implements org.opensearch.ml.common.parameter.Output {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about rename the class name to avoid full class path org.opensearch.ml.common.parameter.Output as they have same class name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

case Output.Entity.FIELD_NEW_VALUE:
parser.nextToken();
entity.setNewValue(parser.doubleValue());
break;
Copy link
Collaborator

@ylwu-amzn ylwu-amzn Jan 7, 2022

Choose a reason for hiding this comment

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

Skip children for default case? Same question for other places

                default:
                    parser.skipChildren();
                    break;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: lai <57818076+wnbts@users.noreply.github.com>
ylwu-amzn
ylwu-amzn previously approved these changes Jan 10, 2022
Copy link
Collaborator

@ylwu-amzn ylwu-amzn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the change!

jackiehanyang
jackiehanyang previously approved these changes Jan 19, 2022
@wnbts wnbts dismissed stale reviews from jackiehanyang and ylwu-amzn via 04c7980 January 19, 2022 23:05
@wnbts wnbts merged commit 3ced87b into opensearch-project:main Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants