-
Notifications
You must be signed in to change notification settings - Fork 89
Georgina Tarres CDM Event Model - Termination for Schedules #4001
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
base: master
Are you sure you want to change the base?
Georgina Tarres CDM Event Model - Termination for Schedules #4001
Conversation
CDM Event Model - Termination for SchedulesBackground When a termination applies to a quantity schedule, all the dated values from the effective period onwards should be set to zero. What is being released? The The period from which the change should take effect is determined using the Review Directions Changes can be reviewed in PR: #4001 Note This comment was generated via Rosetta. |
✅ Deploy Preview for finos-cdm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
value: | ||
if item -> date >= effectiveDate then ( | ||
if direction = QuantityChangeDirectionEnum -> Increase then item -> value + changeAmount | ||
else if direction = QuantityChangeDirectionEnum -> Decrease then item -> value + changeAmount |
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.
Is changeAmount
going to include the sign e.g. -33? If not then shouldn't this be item -> value - changeAmount
.
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.
@chrisisla totally agree, thank you for pointing that out. The sign has been fixed in this commit: 8373fe8
UpdateQuantityAmountForEachMatchingQuantity( | ||
item, | ||
change -> quantity, // FilterChangePriceQuantity(priceQuantity, change) -> quantity, | ||
change, // FilterChangePriceQuantity(priceQuantity, change), |
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.
May as well remove the FilterChangePriceQuantity...
comment now.
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.
Hi @chrisisla, I think it’s better to keep it, as the comment is still relevant if the function FilterChangePriceQuantity
becomes operative in the future. I’ve updated it: before it was FilterChangePriceQuantity -> quantity
, and now it just stays at the change
level.
quantity NonNegativeQuantitySchedule (0..1) <"List of NonNegativeQuantitySchedule to update."> | ||
[metadata location] | ||
change NonNegativeQuantitySchedule (0..*) <"List of new NonNegativeQuantitySchedule to use where the units match."> | ||
change PriceQuantity (0..*) <"List of new PriceQuantity to use where the units match."> |
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.
This function is accepting a NnonNegativeQuantitySchedule
as the source data i.e. a quantity, but you have changed the search term to now be a PriceQuantity
i.e. a quantity and a price (and an observable potentially too).
Are we looking to have separate functions to update quantities and prices or is this function going to now be able to update both?
- If it's the former then we should leave the search term as being a
NonNegativeQuantitySchedule
i.e. keep it tied to quantities only. - If it's the latter then we should consider updating the name of the function to show it operates on either a price or a quantity.
I think we just need a bit more clarity on what the overall goal is for these functions. Do we want an all in one PriceQuantity change function, or separate functions for prices and quantities.
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.
Thank you @chrisisla for the comments. I leave the answer we discussed from the CRWG 9th to keep track:
The function input has changed from quantity
to PriceQuantity
because PriceQuantity
corresponds to the type of the higher level (change
) that wraps both the quantity
and its associated effectiveDate
. This is needed to keep the effectiveDate
linked with the quantity
so that the logic contained in the function has the correct effective date for the applicable quantity, ensuring we don’t lose the linkage between the quantity and the corresponding effective date.
As we agreed, I've added a brief explanation in lines 32 and 81 from the code (see commit fcb608f).
What is being released? Comments added in the code. Note This comment was generated via Rosetta. |
What is being released? fixed sign for decrease changed amount Note This comment was generated via Rosetta. |
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.
Thanks for making the changes @gtarres 🙂
The mapper in PR #4012, built by @hugohills-regnosys, should be contributed together with this PR since it includes the mapping of the effective date from FpML to the quantity change primitive instructions. |
No description provided.