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

Simon Cockx Fix cardinality errors #3295

Open
wants to merge 13 commits into
base: 5.x.x
Choose a base branch
from

Conversation

regnosys-prod-user
Copy link
Collaborator

No description provided.

Fix cardinality errors
Copy link

netlify bot commented Dec 11, 2024

Deploy Preview for finos-cdm ready!

Name Link
🔨 Latest commit 466b8a1
🔍 Latest deploy log https://app.netlify.com/sites/finos-cdm/deploys/675983e87b05f000089e35fd
😎 Deploy Preview https://deploy-preview-3295--finos-cdm.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hugohills-regnosys hugohills-regnosys changed the base branch from master to 5.x.x December 11, 2024 14:29
Copy link
Contributor

@tomhealey-icma tomhealey-icma left a comment

Choose a reason for hiding this comment

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

The function, SecurityFinanceCashSettlementAmount does not work for repo for a couple of reasons:

The filter, tradeState -> trade -> tradeLot -> priceQuantity -> quantity, FinancialUnitEnum -> Share only works for Shares on not other units. Also, FinancialUnitEnum is optional so it may not exist.
The function suggest that the repo repurchase price is the collateral value which is also incorrect. The repurchase price is the cashleg principal + interest.
If this function is intended to only work for Security Lending then please change the function name and remove the repurchase price comments.

One other point, the condition, condition ProductIdentifiersMatch:

tradeState -> trade -> tradableProduct -> tradeLot -> priceQuantity -> observable -> productIdentifier = assetPayout -> securityInformation -> security -> productIdentifier

tradeState -> trade -> tradableProduct -> tradeLot -> priceQuantity -> observable -> productIdentifier -> identifier = assetPayout -> securityInformation -> security -> productIdentifier -> identifier

@LionelSG-REGnosys
Copy link
Contributor

@tomhealey-icma

Tom, you make a couple of points here, namely that

The function, SecurityFinanceCashSettlementAmount does not work for repo for a couple of reasons

and

the condition, condition ProductIdentifiersMatch
is overly restrictive and fails in many cases where the identifiers actually match.

These may be valid points however we believe they are beyond the scope of this particular PR which is focused on addressing a specific issue in the DSL on the evaluation of cardinality. In both this PR #3295 and the related one PR #3294, the only change in the function in question is to address this issue with the inclusion of only-element in the evaluation of the FilterQuantityByFinancialUnit function. Your points relate to the business modelling of these functions for some products; you have identified potential issues in the model which have been there for some time.

We would like to proceed with these PRs and fix the underlying DSL issues which occur across the model, as quickly as possible. Could we propose that we:

  • unblock and proceed to release these PRs
  • raise issue(s) related to what you have identified
  • prioritise these through the appropriate WGs to get the issues addressed.

Thanks
Lionel, acting as Release Manager for the CDM.

tomhealey-icma
tomhealey-icma previously approved these changes Jan 6, 2025
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.

5 participants