JEAN BAPTISTE ZIADE update initial PR cf Comments in PR 3366 #3637
Conversation
|
This is a follow on from #3366. The master branch has moved on since the initial contribution so a second PR was created. |
…nds of unwinds partial full ...
|
What is being released? have modified enum EventIntentEnum to better distinguished between kinds of unwinds (partial, full before maturity, full at maturity), then have modified as well enum FeeTypeEnum for harmonization purposes Note This comment was generated via Rosetta. |
|
FYI - in last Contrib just sent today, have modified these two types :
but since there are still issues when pushing Contrib, kindly check you can see these last changesd ? if Contrib has worked, you should see as below : |
| @@ -7801,7 +7801,7 @@ synonym source FpML_5_Confirmation_To_TradeState extends FpML | |||
| [value "AssignmentFee"] | |||
| + Increase | |||
| [value "IncreaseFee"] | |||
| + PartialTermination | |||
There was a problem hiding this comment.
java code fails to compile as this field is renamed , changes would have to applied to fix this
|
@lolabeis thanks |
There was a problem hiding this comment.
The extra changes to FeeIntentEnum and EventIntentEnum (renaming / splitting of enum values for Decrease) were not part of the originally proposed design, and do not appear related to corporate action. As raised by the maintenance team, they also bear some negative downstream impact which may delay this contribution.
The recommendation is to take that change out of this PR and raise a separate issue for it, to be discussed at the CRWG.
Also the CRWG should opine on the recommended removal of PositionEventIntentEnum, which is being fused into EventIntentEnum.
Further detailed review comment provided below.
| tenorTillMaturity number (1..1) <"The duration between last fixing date and the payment date of accruals, calculated in accordance with the appropriate DayCountFraction."> | ||
| dayCountFraction DayCountFractionEnum (1..1) <"The enumerated values to specify the day count fraction."> | ||
| shortStubInterpolationRate FloatingRate (0..1) <"Describes the rate and related tenor for the short stub when the accrualRate optionnaly results from an interpolation method."> | ||
| longStubInterpolationRate FloatingRate (0..1) <"Describes the rate and related tenor for the long stub when the accrualRate optionnaly results from an interpolation method."> |
There was a problem hiding this comment.
The previous suggestion was to use FloatingRateIndex (small object) for these 2 attributes, not FloatingRate (larger)
There was a problem hiding this comment.
OK, all in all i agree but anyway realized had to refactor this part because what needs to be represented is the stub tenor, and also the fact you migh use either floating rate, or given rate separatly agreed... so cannot do this no,w but will refactor type AccrualFactorCalculationTerms - please check it in next contrib
|
I would like @mgratacos and team to opine on the removal of |
|
dully understand rationale why "extra-change" (out of initial scope driven by corporate action refactoring) should be reviewed by CRWG main cause for that being in my view because we also propose in PR, as you suggested it, a bigger extra-change than the ones have initiall^y proposed, that is removal of background : "extra-changes" were induced by other prior changes in i mean, all in all, let's present to CRWG all these changes as a whole, provide context
|
No. The fact that the same EventIntentEnum is being touched in the present PR is irrelevant. This is new issue and as such should go through the triage queue before being aired at the CRWG. The next CRWG is quite packed already with many issues to get through first, so let's not shoe-horn a different one. Please present a PR that is ready to be reviewed for the specific issue that was previously approved, to allow your final change to be ratified at the next CRWG. |
…tEnum so for these two types...
|
What is being released? removed changes which impacted PositionEventIntentEnum and EventIntentEnum, so for these two types, should be no change wirh regards to current CDM Prod v5 Note This comment was generated via Rosetta. |
|
replaced with #3642 |
|
@JBZ-Fragmos |
|
Note for @mgratacos and @manel-martos: |
|
What is being released? removed changes in FeeTypeEnum (so should be re-aligned with CDM Prod v5) Note This comment was generated via Rosetta. |
|
Oops, yes, sorry, have just sent contrib for removing any changes in FeeTypeEnum
De : lolabeis ***@***.***>
Envoyé : lundi 14 avril 2025 14:48
À : finos/common-domain-model ***@***.***>
Cc : Jean-Baptiste Ziadé ***@***.***>; Mention ***@***.***>
Objet : Re: [finos/common-domain-model] JEAN BAPTISTE ZIADE update initial PR cf Comments in PR 3366 (PR #3637)
@JBZ-Fragmos<https://github.com/JBZ-Fragmos>
It seems that you left an enum change in FeeTypeEnum that should have been removed too. I also posted detailed inline comments in my previous review, some of them still need to be addressed.
Could you please resubmit accordingly - ideally before tomorrow's WG so we can get it ratified there?
-
Reply to this email directly, view it on GitHub<#3637 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWFL2PKOFPVAPN6MKWGYFSL2ZOU7RAVCNFSM6AAAAAB235GBT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMBRGYYDCNJRHA>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
[https://avatars.githubusercontent.com/u/29451984?s=20&v=4]lolabeis left a comment (finos/common-domain-model#3637)<#3637 (comment)>
@JBZ-Fragmos<https://github.com/JBZ-Fragmos>
It seems that you left an enum change in FeeTypeEnum that should have been removed too. I also posted detailed inline comments in my previous review, some of them still need to be addressed.
Could you please resubmit accordingly - ideally before tomorrow's WG so we can get it ratified there?
-
Reply to this email directly, view it on GitHub<#3637 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWFL2PKOFPVAPN6MKWGYFSL2ZOU7RAVCNFSM6AAAAAB235GBT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMBRGYYDCNJRHA>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
Thanks @JBZ-Fragmos. Please also address outstanding inline review comments and provide detailed release notes (as requested by @PayalKhanna on the Release PR) in comment. |
|
Release Note is drafted in #3642 |
|
Approved at the CDM Contribution Review Working Group - April 15th, 2025, provided the outstanding review comments (inline) are addressed. |
|
@lolabeis however, in constext where stub rates are supposed to represent the terms of the calculation for the interpolation rate, no besides, i've noticed there may be a need to optionnally represent the exact duration of stub period (notably when the intent is to document the calculation details, then in accordance with above problem statement
beleive there is benefit at both levels :
|
…es also add optional attribu...
|
@lolabeis |
|
@JBZ-Fragmos Thanks for your latest commit. The only outstanding question relates to stub (I've marked the other comments as resolved).
Reusing the existing Also each Eventually, this suggests a more appropriate implementation using Your condition that both long and short should exist could be added directly in |
|
I agree with your last proposal in terms of type structure, only the name for component well just to say right name should be explicit also when reading it with description, it makes sense : interpolationTerms StubValue (0..1) <"Describes the rate, tenor, period duration for the short and long stubs, when the accrualRate optionnaly results from an interpolation method."> |
|
actually, there is still one issue to solve for re-using properly the indeed, with current as a result, you can see that one use case is obviously missing, that is "resolvable" method using fixed rates i.e. when the short stub rate and the long stub rate are given as "resolved", for being used to calculate a "resolvable" interpolation rate for now stubRate exists inside StubValue but this component would corresponds to the "resolved" interpolation rate itself, also cardinality is bouded to 1, so we cannot re-use this component as such for the purpose of modelling interpolation composed of two fixed rates... also we cannot simply add straight attribute such as interpolationFixedRate number (0..2) for instance, because this does not permit to associate the Period per each of the two rates... so as a conclusion, have enriched
see below changes being highlighted : overall, for me the whole structure looks well balanced, and very readable - see below how this looks like : |
…nterpolationFixedRate 02 ver...
|
What is being released? enriched StubValue as below :
Note This comment was generated via Rosetta. |
|
@JBZ-Fragmos Thanks for the analysis. Overall you're making sensible suggestions regarding stub. However, I'm now a little uncomfortable reviewing and approving those additional changes, given that this latest scope creep wasn't part of the design originally discussed and approved by the CRWG. Enabling the stub component to handle interpolation between fixed rates has potential application beyond the corp action use case and, as such, is worthy of proper scrutiny. Therefore my recommendation is to progress with your changes where A separate issue should be raised to deal specifically with the fixed-rate interpolation case for the stub component. +@llynhiavu FYI. |
|
@leo ***@***.***>
It is a bit similar with discussion we've had recently about changes in EventIntentEnum that then I have removed, but here the link is really deeper, I mean yes, StubValue would be changed, but that is directly a result of properly improving the representation of CorporateAction
You cannot avoid that when updating "X" or creating new set of components for modelling "X", possibly this creates refactoring also for another existing "Y" component - and I would say, that is most often the case in practice (case where you add fully new type without impacting any prior existing one is really an exception)
You are reasoning as if we could proceed with CorpAction enrichement without having fixed rate interpolation embedded into it, and you are suggesting we proceed it separately
But why, eventually ?
* First, it means we release partial representation of original scope (because as a fact, some CorpAction result in accrual rate calculated with an interpolation of fixedRate, and that is not an exception, in practice, that is the most likely case because duration at stake until maturity might be quite long, so stub rates used are not straight observable ones in rate curves, so party are actually calculating them as bespoke values, meaning from our standpoint related value into CDM is resolved as "fixedRate")
* Second, the change incurred in StubValue does not impact existing structures : we are adding one component with a required choice compared to prior existing attributes, hence enriching an prior existing type, induced by precise use case of CorpAction (as I'm always being told "what is use case ?" here we have it, very clear, so why in such case we should dis-connect it ?)
De : lolabeis ***@***.***>
Envoyé : jeudi 17 avril 2025 16:30
À : finos/common-domain-model ***@***.***>
Cc : Jean-Baptiste Ziadé ***@***.***>; Mention ***@***.***>
Objet : Re: [finos/common-domain-model] JEAN BAPTISTE ZIADE update initial PR cf Comments in PR 3366 (PR #3637)
@JBZ-Fragmos<https://github.com/JBZ-Fragmos> Thanks for the analysis.
Overall you're making sensible suggestions regarding stub. However, I'm now a little uncomfortable reviewing and approving those additional changes, given that this latest scope creep wasn't part of the design originally discussed and approved by the CRWG. Enabling the stub component to handle interpolation between fixed rates has potential application beyond the corp action use case and, as such, is worthy of proper scrutiny.
Therefore my recommendation is to progress with your changes where StubValue is used "as-is" in AccrualFactorCalculationTerms (with your proposed attribute name: interpolationTerms). This is a good incremental change in relation to corp action to release in CDM 7.
A separate issue should be raised to deal specifically with the fixed-rate interpolation case for the stub component.
***@***.***<https://github.com/llynhiavu> FYI.
-
Reply to this email directly, view it on GitHub<#3637 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWFL2PMYNJWWWVA3GE6PKTT2Z63GVAVCNFSM6AAAAAB235GBT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJTGEZTCNBWGY>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
[https://avatars.githubusercontent.com/u/29451984?s=20&v=4]lolabeis left a comment (finos/common-domain-model#3637)<#3637 (comment)>
@JBZ-Fragmos<https://github.com/JBZ-Fragmos> Thanks for the analysis.
Overall you're making sensible suggestions regarding stub. However, I'm now a little uncomfortable reviewing and approving those additional changes, given that this latest scope creep wasn't part of the design originally discussed and approved by the CRWG. Enabling the stub component to handle interpolation between fixed rates has potential application beyond the corp action use case and, as such, is worthy of proper scrutiny.
Therefore my recommendation is to progress with your changes where StubValue is used "as-is" in AccrualFactorCalculationTerms (with your proposed attribute name: interpolationTerms). This is a good incremental change in relation to corp action to release in CDM 7.
A separate issue should be raised to deal specifically with the fixed-rate interpolation case for the stub component.
***@***.***<https://github.com/llynhiavu> FYI.
-
Reply to this email directly, view it on GitHub<#3637 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWFL2PMYNJWWWVA3GE6PKTT2Z63GVAVCNFSM6AAAAAB235GBT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJTGEZTCNBWGY>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
|
@JBZ-Fragmos So you have 2 options:
I'm advocating for 2 because it registers a first win more quickly, especially considering how busy the CRWG queue is already. Regardless of the chosen option and given the broader application of the 'stub' component, I would recommend a separate PR for it with its own release notes anyway. |
|
What is being released? have removed prior changed proposed for type StubValue Note This comment was generated via Rosetta. |
|
OK, then will follow your advice, let's go for "n°2"
please confirm you have all your need to merge PR as it is now ?
please ensure this one is processed with relevant status in "queue" thank you |
Supersedes #3366