Attempted to fix the Projection Upper band and lower band calculation#329
Attempted to fix the Projection Upper band and lower band calculation#329abhishekmittal15 wants to merge 1 commit intocinar:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #329 +/- ##
==========================================
+ Coverage 64.64% 66.00% +1.36%
==========================================
Files 93 93
Lines 1533 1562 +29
Branches 183 186 +3
==========================================
+ Hits 991 1031 +40
+ Misses 529 518 -11
Partials 13 13
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Hi @cinar , I was wondering if you got a chance to have a look at the previous comment and the code? Regards, |
|
My sincere apologies, I totally missed your earlier message. Let me quickly take a look. |
cinar
left a comment
cinar
left a comment
There was a problem hiding this comment.
I added few comments. Thank you very much for your contribution!
| } | ||
|
|
||
| // Just pads a number array with zeros to the right so that the length of the arr becomes desiredLength | ||
| function padWithZeros(desiredLength: number, arr: number[]) { |
There was a problem hiding this comment.
This is a very nice helper function to add to ../../helper/numArray probably?
| const arr_period: number[] = generateNumbers(0, period, 1); | ||
| const pu = new Array(closings.length); | ||
| for (let i = 0; i < highs.length; i++) { | ||
| // for (let j = 0; j < period; j++) { |
|
|
||
| const pl = new Array(closings.length); | ||
| for (let i = 0; i < lows.length; i++) { | ||
| // for (let j = 0; j < period; j++) { |
|
|
||
| const pu = mmax(period, vHighs); | ||
| const pl = mmin(period, vLows); | ||
| // const pu = mmax(period, vHighs); |
| return result; | ||
| } | ||
|
|
||
| const closings = generateRandomNumbers(100, 30, 5); |
There was a problem hiding this comment.
I guess some test code got left over here?
| return mean + stdDev * (Math.random() - 0.5); | ||
| } | ||
|
|
||
| function generateRandomNumbers(count: number, mean: number, stdDev: number) { |
There was a problem hiding this comment.
May be the function can be named differently to reflect how you are using the mean and stdDev? I guess they are not just random number right? Also would be nice to have this in ../../helper/numArray.ts as well.
| const highs = addBy(0.5, closings); | ||
| const lows = subtractBy(0.5, closings); | ||
|
|
||
| const result = projectionOscillator(14, 5, highs, lows, closings); |
There was a problem hiding this comment.
Since you have a test case here, can we put it into the unit test?
|
So @cinar , does the changes in the logic part make sense. Does the error in the previous version of the code and my fix make logical sense, or do you have any questions about it. Yes I will clean up the file, I just wanted to first confirm if the logic is right 😅 . Also is there some way to check if this implementation is actually giving the right values? |
|
I think your fix definitely makes sense. What we are lacking is a test case here. If we can find a source of truth for this calculation that we can compare the results against, it will give us more confidence that this is the correct algorithm to go with. Not sure if there is already an example that we can use, or an other tool to compute and compare the results against? |
3 participants
Attempting to fix #218
Modified the projection Oscillator function by changing the calculation of the arrays
puandplaccording to what I suspect the issue is, as I mentioned in the conversation of #218Hi @cinar ,
Please have a look at the code which has
LOOK HEREwritten, that is the changes made to the implementation mainly.Let me know what you think about it.
The file is messy right now, I will clean it after you have approved the changes in the logic part of the code.
I also needed advice on how I can test this indicator.
Thanks