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

Include score functionality proposal #104

Conversation

fernandopradocabrillo
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature
  • documentation

What this PR does / why we need it:

Include scoring functionality. When a property match results in false, the api will also return a value representing how similar is the input to the information stored by the Operator.

Update the yaml structure to improve code reusability.

Which issue(s) this PR fixes:

Fixes #85
Fixes #96

Special notes for reviewers:

Changelog input

 ### Added
- Include score functionalily

@GillesInnov35
Copy link
Collaborator

hi @fernandopradocabrillo, thanks for this PR

  • we think also at Orange that score match functionality should,be proposed in KYC Match as soon as possible.
  • do we have to complete the resource match of KYC Match v0.1.0 while maintaining the backward compatibility
  • we might propose 2 endpoints to avoid a design complexity with by addition of score match attributes

Gilles

@GillesInnov35
Copy link
Collaborator

GillesInnov35 commented Jul 1, 2024

@fernandopradocabrillo, all, we discussed internally at Orange on the design proposal concerning new optional score match attributes for some of attributes of KYC match v0.1.0 and the option to have 2 end points.

  • in conclusion we validate the design proposed by you in the PR including new score attributes.
  • One more comment: to facilitate development it would be useful to add in API specifications that a score result will always be returned when match result is equal to False for the concerned attributes. Otherwise it is an error.
    Thanks
    BR
    Gilles

@ToshiWakayama-KDDI
Copy link
Collaborator

Hi @fernandopradocabrillo ,

Thank you for the proposal. Due to my sickness and so on, we are stil looking into the yaml, but we have a clear comment for clarification now.

Based on our previous discussion, it was commonly agreed to keep backward Compatiblity to V0.1 and to have Match Scoring function optional. From KDDI side, it is understood that if Match Scoring function is not available (e.g. has not been implemented), it is ok not to return MatchScoreResult even if a MatchResult is false.

This may be different from one of Gilles comments, so, I am quickly commenting this.

Other parts are still investigated.

many thanks,
Toshi

@GillesInnov35
Copy link
Collaborator

hi @ToshiWakayama-KDDI, thanks for your comment.

Based on our previous discussion, it was commonly agreed to keep backward Compatiblity to V0.1 and to have Match Scoring function optional. From KDDI side, it is understood that if Match Scoring function is not available (e.g. has not been implemented), it is ok not to return MatchScoreResult even if a MatchResult is false.

You're right my proposition doesn't work in that case. Unfortunately as we'll have a unique endpoint for the 2 versions (which are quite different I think) we can't associate a mandatory score attribute when False result has been returned. More rules should be developed to process the response on consumer side.

BR
Gilles

@ToshiWakayama-KDDI
Copy link
Collaborator

Hi @GillesInnov35 ,

Thanks, Gilles. Could you elaborate "we'll have a unique endpoint for the 2 versions (which are quite different I think)", please? I may have misunderstood your comments.

Many thanks,
Toshi

@GillesInnov35
Copy link
Collaborator

Toshi,

Could you elaborate "we'll have a unique endpoint for the 2 versions (which are quite different I think)",

I just mean that it'd have been easier to have 2 endpoints as we see that

  • KYC Match v0.1.0 and v0.2.0 will be integrated in the Meta Release of September (to be confirmed)
  • score match functionality (v0.2.0) is not available in all countries - new process rule should be added on consumer dev.

But never mind, sure it'll work
Thanks
Gilles

@fernandopradocabrillo
Copy link
Collaborator Author

  • KYC Match v0.1.0 and v0.2.0 will be integrated in the Meta Release of September (to be confirmed)

@GillesInnov35 I don't get this one, only v0.2.0 will go to the Meta Release I think. We cannot propose 2 versions.

@ToshiWakayama-KDDI
Copy link
Collaborator

Hi @GillesInnov35 , @fernandopradocabrillo , all,

Thank you, Gilles. A bit difficult for me to fully understand. I need to discuss with my colleagues, I guess.

In the meantime, I would like to share our view. In one of my previous comments, I proposed an idea that Match Result Score will not be returned if the match scoring function is not available. From KDDI side, we would like to keep this.

One reason is that KDDI does not plan to implement the match scoring function because our customers in Japan do not need it. No customer demands. So, we will keep the v0.1 base minimum function, i.e. returning True/False MatchResults only.

Apart from KDDI's own context, we think that not all customers want to receive Match Result Score automatically when Match Result is false, and some API customers may be confused about how to deal with it. Also, for operators, some operators do not want to return Match Result Scores, like KDDI. Operator's implementation is one important point, we think, and we should take a direction to make this API easy to implement to operators and to accelerate this API's implementation by global operators. For this, we could define simple v0.1 True/False MatchResult function as the basic function or MVP?, which is kind of mandatory function, and define the Match Scoring function as an enhanced, optional function, which can be implemented if needed by each operator.

Sorry for the long sentence, but the above is some addtional comments to KDDI' original view that Match Scoring should be optional.

What do you think?

Best regards,
Toshi
KDDI

@KevScarr
Copy link
Collaborator

KevScarr commented Jul 4, 2024

@ToshiWakayama-KDDI My understanding is that the score is an optional attribute. So it is down to the MNO to either implement it or not implement it. I would suggest it's consistent within an MNOs response, ie if implemented it's always available for 'false' responses so there is consistency.
At a market/country level, the MNOs can work together to agree to all provide it or not based on their customers needs (as they would all have common customers, so the ask should be the same) ??

@GillesInnov35
Copy link
Collaborator

hi @fernandopradocabrillo,

@GillesInnov35 I don't get this one, only v0.2.0 will go to the Meta Release I think. We cannot propose 2 versions.

yes you're right, only version v0.2.0 will be published in Meta Release. My sentence was not clear.
I wanted to say that functionalities of v0.1.0 and evolutions with v0.2.0 will be published in the Meta Release.

I think that we all agree that scoring could be part of Meta Release but must be optional as it is not available in all countries.

Thanks a lot
BR
Gilles

@claraserranosolsona
Copy link

Hi @GillesInnov35 ,

Can you review and approve the PR #104?

Once you do it, we will include the following comment and we will create the RC v0.2.0 as agreed in last meeting
"Unless otherwise captured in the specification, score will use the JaroWinkler distance algorithm for all countries"

Regards,
Clara

@ToshiWakayama-KDDI
Copy link
Collaborator

Hi @claraserranosolsona , @fernandopradocabrillo , all

Thanks for Kana parameters being included for Scoring.

Please wait for sometime for my approval, because we are still checking the YAML proposal. Sorry for the delay, but we should be ready by tomorro's KYC SP meeting.

Thank you for your understanding.
Toshi

@ToshiWakayama-KDDI
Copy link
Collaborator

Hi @claraserranosolsona , @fernandopradocabrillo , all,

The proposed v0.2 YAML looks good to us, but just one thing to confirm. V0.2 is backward compatible with v0.1. Correct?

In addition, please let us confirm the following understanding is correct.

  1. v0.1 Match basic function; returning value True/False/not-available
    -Attributes are all optional, so, if an operator does not have specific attributes, it can send responses 'not-available' for match request of those unavailable attributes.

  2. v0.2 Match score added: Optional feature returning match score calculated by Jaro-Winkler algo (or by other algo, if applicable), for string attributes.
    -Score is an optional feature, so, it is up to MNO to implement the feature.
    ->If implemented, when False is returned for a string attribute, (attribute)MatchScore:XX(%) should be returned.
    ->If not implemented, (attribute)MatchScore is not returned.

  3. For KYC Match, v0.2 (including Scoring) will be included in the Fall24 Meta-release.

  4. For string match score algorithm, default algorithm will be Jaro-Winkler distance algorithm, however, if any region/country/operator decide to use a different algorithm, it would be possible. For this, "Unless otherwise captured in the specification, score will use the Jaro-Winkler distance algorithm for all countries." is put in the API description, and in future the chosen algorithm should be written in the API description.

Many thanks,
Toshi
KDDI

@ToshiWakayama-KDDI
Copy link
Collaborator

This PR is to be merged as agreed in the meeting 2024/07/09 and supported by three codeowners.

@ToshiWakayama-KDDI
Copy link
Collaborator

Hi @GillesInnov35 ,

Can you review and approve the PR #104?

Once you do it, we will include the following comment and we will create the RC v0.2.0 as agreed in last meeting "Unless otherwise captured in the specification, score will use the JaroWinkler distance algorithm for all countries"

Regards, Clara

To note, the above will be taken care of by another PR.

Thanks.
Toshi

@ToshiWakayama-KDDI ToshiWakayama-KDDI merged commit 341fefe into camaraproject:main Jul 10, 2024
@@ -276,162 +280,127 @@ components:
- FEMALE
- OTHER

MatchResult:
type: string
enum:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we think about additional result value indicating that the match was false, but the score can not be computed. There are implementations based on hashed values of properties and then the scoring algorithm does not work.
Another value can be like: 'false_no_score'

Copy link
Collaborator

Choose a reason for hiding this comment

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

hi @rartych, do you mean that in some case some attribute such as givenName or familyName provided in the KYC-Match request could be hashed value ?
Thanks
Gilles

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, the matching can be against hashed values and then the score can not be computed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @rartych , @jgarciahospital , all

We have not discussed hashed values for KYC Match so far, so, my understanding is that all discussions we had are based on non-hashed values. I would suggest we could consider hashed values in future releases, for which there has been Issue #86 already.

Best regards,

Copy link
Collaborator

Choose a reason for hiding this comment

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

My issue is: what should be returned when there is no matching but the score can not be calculated - we can decide to recommend returning MatchScoreResult=0, but it can be misleading. Or the nullable: true is the solution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hi @rartych , as MatchScoreResult is optional, if it should be not returned, is it right ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, is it valid to return MatchResult=false and not return MatchScoreResult at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes - MatchScoreResult is an optional return attribute.

Choose a reason for hiding this comment

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

Yes, this is the proposal

This parameter is optional, it will not be returned IF the operator decides not to implement the score property for any attribute OR IF the operator implements the score property but, for whatever reason, it is not possible to calculate it for a specific attribute based on the client's input or the related operator information. For these two cases, the score property won't be returned in the response.

@ToshiWakayama-KDDI
Copy link
Collaborator

hi Toshi, it has been already done Gilles

Hi @GillesInnov35 ,

Sorry, what has been already done, do you mean?

For the sentence "Unless otherwise captured in the specification, score will use the JaroWinkler distance algorithm for all countries", I cannot find it in the current kyc-match.yaml...

Thanks,
Toshi

@jgarciahospital
Copy link
Collaborator

hi Toshi, it has been already done Gilles

Hi @GillesInnov35 ,

Sorry, what has been already done, do you mean?

For the sentence "Unless otherwise captured in the specification, score will use the JaroWinkler distance algorithm for all countries", I cannot find it in the current kyc-match.yaml...

Thanks, Toshi

Hi toshi, it is included in #111

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.

Create a common enum for attributeMatch result [KYC Match] Scoring
7 participants