Fix bugs in TimeDiscretization and enhance TimeDiscretizationInterface

delreluca commented 6 years ago

See finmath-lib issue #58

We unify duplicate removal and tick size rounding in TimeDiscretization Building upon that we define unions and intersections of discretizations

What is this PR for?

Unifying the handling of tick sizes and duplicates in the TimeDiscretization class in order to add unions and intersections to the TimeDiscretizationInterface.

What type of PR is it?

Bug Fix, Refactor and Feature

Todos

I would suggest revisiting retrieving the default tick size from a system property, as now it can be set when constructing the TimeDiscretization. It makes the thing harder to test and it might clash when two dependencies both use finmath with different tick sizes.

What is the related issue?

https://github.com/finmath/finmath-lib/issues/58

How should this be tested?

Unit tests are provided

Screenshots

Questions:

Does the licenses files need update?

NO

Are there breaking changes for older versions?

YES If there was any reliance on duplicates or unrounded time points. I ran the whole unit test suite and it didn’t seem to be the case there.

Does the change require additional documentation?

NO

cfries commented 6 years ago

I had a very quick look, but looks to me as if the old constructor did a sort, but the new one does not. This might be an issue. Will check asap.

delreluca commented 6 years ago

I will adjust the design changes later this day (asterisk import, tabs, missing Javadoc).

Regarding the tick sizes: I agree that it should be min for unions and max for intersections. I’m not sure how to reliably enforce that the tick sizes are integer multiples of each other for doubles, I guess I will not check and warn about it in the Javadoc like this: “If the tick sizes are not integer multiples of each other multiple operations might alter the time points, for example a.intersect(B.union(a)) might be different from a“.

What do you think about it?

Another possibility would be to remember the unrounded time points, but I guess at that point the user could remember it themselves and re-create a TimeDiscretization with a common tick size.

cfries commented 6 years ago

Your solution is fine. Remembering the original points sound good, but looks wasteful to me… lets don’t do it. What do you thing about requiring tick sizes to be equal? The tick side is like a unit and you can union „days with days“ and „years with years“ but you are not allow to mix. (I would bet that we do not have the use case of different tick size, because here the tick size is just to have a robust definition of „equal“ the floating point times.

Am 13.07.2018 um 08:48 schrieb delreluca notifications@github.com:

I will adjust the design changes later this day (asterisk import, tabs, missing Javadoc).

Regarding the tick sizes: I agree that it should be min for unions and max for intersections. I’m not sure how to reliably enforce that the tick sizes are integer multiples of each other for doubles, I guess I will not check and warn about it in the Javadoc like this: “If the tick sizes are not integer multiples of each other multiple operations might alter the time points, for example a.intersect(B.union(a)) might be different from a“.

What do you think about it?

Another possibility would be to remember the unrounded time points, but I guess at that point the user could remember it themselves and re-create a TimeDiscretization with a common tick size.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/60#issuecomment-404742623, or mute the thread https://github.com/notifications/unsubscribe-auth/ADo8HzudGebaQzWHcas9bMC7eVxV6t5Kks5uGEK8gaJpZM4VNrBZ.

delreluca commented 6 years ago

I think requiring the same tick size too restrictive. In many cases users will not bother setting a non-default tick size. But if they do, they will have to research these edge cases anyway, and they will find the answer in the Javadoc.

Here is an example. Someone might reduce the tick size to do an intraday volatility simulation with GARCH, but they want to use the simulated volatilities in a Euler scheme with a coarser time grid where they don’t specify a tick size. (Somewhat contrived example I admit) On Fri 13. Jul 2018 at 09:11, Christian Fries notifications@github.com wrote:

Your solution is fine. Remembering the original points sound good, but looks wasteful to me… lets don’t do it. What do you thing about requiring tick sizes to be equal? The tick side is like a unit and you can union „days with days“ and „years with years“ but you are not allow to mix. (I would bet that we do not have the use case of different tick size, because here the tick size is just to have a robust definition of „equal“ the floating point times.

Am 13.07.2018 um 08:48 schrieb delreluca notifications@github.com:

I will adjust the design changes later this day (asterisk import, tabs, missing Javadoc).

Regarding the tick sizes: I agree that it should be min for unions and max for intersections. I’m not sure how to reliably enforce that the tick sizes are integer multiples of each other for doubles, I guess I will not check and warn about it in the Javadoc like this: “If the tick sizes are not integer multiples of each other multiple operations might alter the time points, for example a.intersect(B.union(a)) might be different from a“.

What do you think about it?

Another possibility would be to remember the unrounded time points, but I guess at that point the user could remember it themselves and re-create a TimeDiscretization with a common tick size.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/finmath/finmath-lib/pull/60#issuecomment-404742623>, or mute the thread < https://github.com/notifications/unsubscribe-auth/ADo8HzudGebaQzWHcas9bMC7eVxV6t5Kks5uGEK8gaJpZM4VNrBZ .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/60#issuecomment-404747012, or mute the thread https://github.com/notifications/unsubscribe-auth/AiO6c-fWgc9Xkkdmql7hceioJFnHyzhbks5uGEgfgaJpZM4VNrBZ .

cfries commented 6 years ago

Another thought on this:

Since tickSize is used to define the equivalent classes of „times“ I was thinking about the need to change it. Maybe someone going intraday and someone going for end of day valuation would both be satisfied with „times are equal if they are equal up to the milli-second. That is tickSize is 1.0 / (365.0 24.0 60.0 60.0 1000.0). This is 3E10, leaving 1E5 years as the the highest possible time horizon. But since we are using IEEE754, best would be to have tickSize being a rounding of the mantissa only (which could be 10^-12 then). …

Am 13.07.2018 um 12:12 schrieb delreluca notifications@github.com:

I think requiring the same tick size too restrictive. In many cases users will not bother setting a non-default tick size. But if they do, they will have to research these edge cases anyway, and they will find the answer in the Javadoc.

Here is an example. Someone might reduce the tick size to do an intraday volatility simulation with GARCH, but they want to use the simulated volatilities in a Euler scheme with a coarser time grid where they don’t specify a tick size. (Somewhat contrived example I admit) On Fri 13. Jul 2018 at 09:11, Christian Fries notifications@github.com wrote:

Your solution is fine. Remembering the original points sound good, but looks wasteful to me… lets don’t do it. What do you thing about requiring tick sizes to be equal? The tick side is like a unit and you can union „days with days“ and „years with years“ but you are not allow to mix. (I would bet that we do not have the use case of different tick size, because here the tick size is just to have a robust definition of „equal“ the floating point times.

Am 13.07.2018 um 08:48 schrieb delreluca notifications@github.com:

I will adjust the design changes later this day (asterisk import, tabs, missing Javadoc).

Regarding the tick sizes: I agree that it should be min for unions and max for intersections. I’m not sure how to reliably enforce that the tick sizes are integer multiples of each other for doubles, I guess I will not check and warn about it in the Javadoc like this: “If the tick sizes are not integer multiples of each other multiple operations might alter the time points, for example a.intersect(B.union(a)) might be different from a“.

What do you think about it?

Another possibility would be to remember the unrounded time points, but I guess at that point the user could remember it themselves and re-create a TimeDiscretization with a common tick size.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/finmath/finmath-lib/pull/60#issuecomment-404742623>, or mute the thread < https://github.com/notifications/unsubscribe-auth/ADo8HzudGebaQzWHcas9bMC7eVxV6t5Kks5uGEK8gaJpZM4VNrBZ .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/60#issuecomment-404747012, or mute the thread https://github.com/notifications/unsubscribe-auth/AiO6c-fWgc9Xkkdmql7hceioJFnHyzhbks5uGEgfgaJpZM4VNrBZ .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/60#issuecomment-404790381, or mute the thread https://github.com/notifications/unsubscribe-auth/ADo8H4Lx_JkKZuOe6-w6BqvTFQ5uXWsvks5uGHKggaJpZM4VNrBZ.

delreluca commented 6 years ago

Hope the second commit resolves the issues.

Regarding changing the default to 1 millisecond, we could do that, but I guess that would be a different issue. By the way I’d also reconsider getting the default from the system property, as this seems a bad practice if the library is used as a dependency by two clients.

cfries commented 6 years ago

The properties file is an integral part of the distributed JA and a user has to actively overwrite that property. Why bad practice?

Am 13.07.2018 um 21:19 schrieb delreluca notifications@github.com:

Hope the second commit resolves the issues.

Regarding changing the default to 1 millisecond, we could do that, but I guess that would be a different issue. By the way I’d also reconsider getting the default from the system property, as this seems a bad practice if the library is used as a dependency by two clients.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/60#issuecomment-404928594, or mute the thread https://github.com/notifications/unsubscribe-auth/ADo8H6kpwQYocWtMmnD-Dw749TAJtRxJks5uGPKkgaJpZM4VNrBZ.

delreluca commented 6 years ago

Consider a Java program that depends on two other Java libraries A and B each depending on finmath. They run on the same JVM. A sets the default tick size to 1 ms, but B sets it to 10 ms. Now the default tick size depends on the order A’s and B’s setProperty calls are executed.

cfries commented 6 years ago

I see the point.

Am 13.07.2018 um 22:44 schrieb delreluca notifications@github.com:

Consider a Java program that depends on two other Java libraries A and B each depending on finmath. They run on the same JVM. A sets the default tick size to 1 ms, but B sets it to 10 ms. Now the default tick size depends on the order A’s and B’s setProperty calls are executed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/60#issuecomment-404948946, or mute the thread https://github.com/notifications/unsubscribe-auth/ADo8H6WSVEHd2oLNkvGbTkf3hqmKHI78ks5uGQa2gaJpZM4VNrBZ.

cfries commented 6 years ago

I would favour a hard coded default, now that tickSize can be changed at construction.

Am 13.07.2018 um 22:48 schrieb Christian Fries email@christian-fries.de:

I see the point.

Am 13.07.2018 um 22:44 schrieb delreluca <notifications@github.com mailto:notifications@github.com>:

Consider a Java program that depends on two other Java libraries A and B each depending on finmath. They run on the same JVM. A sets the default tick size to 1 ms, but B sets it to 10 ms. Now the default tick size depends on the order A’s and B’s setProperty calls are executed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/60#issuecomment-404948946, or mute the thread https://github.com/notifications/unsubscribe-auth/ADo8H6WSVEHd2oLNkvGbTkf3hqmKHI78ks5uGQa2gaJpZM4VNrBZ.

delreluca commented 6 years ago

Me too, it fits with the new constructors. I’m not sure whether we should do it in this PR, though. If we change that line we might as well also change the default value itself as already suggested.

What’s your take on how changing the default (to 1 ms for example) might break existing code?

cfries commented 6 years ago

With respect to changing the default: My rule is that for revision a.b.c a change in c does not break code, a change in b does not break interfaces, but may lead to changes in behaviour and a change in a has massive changes in interfaces. Changing the default is fine, but I would do a 4.3.0 release then. (Should be in a separate PR). (Note: I have violated my own rule, but I try comply with it in the future).

finmath / finmath-lib