Add VWAP Quarterly and Yearly periods#77
Conversation
|
Thanks for the contribution. Adding quarterly and yearly VWAP periods is a reasonable feature request and fits the existing indicator well. That said, I think the implementation needs one more pass before merge. Weekly and monthly logic in this codebase already relies on existing period/session-aware helpers, while this PR introduces new quarter/year boundaries with separate local logic. I am not yet confident that the new behavior will stay fully consistent with session handling and the rest of the indicator's reset rules. The feature itself makes sense, but I would prefer to see the implementation rebased onto the current code and validated against the existing session model before merging. |
de46096 to
55f5ecf
Compare
|
Ah good to know! I have just rebased :) And how could I do that validation? Could you please point me to an example so I can see how it is done? I added those 2 methods to mimic what I think there was in the base class, but I did not find the code for it either |
|
The validation I had in mind is mostly behavioral validation against the existing period logic, rather than a separate test framework. A few pointers from this repo:
So the concern is not that your Also, the reason you probably could not find the original implementation is that What I would validate manually:
So in short: I think your implementation is a reasonable first step, but I would be more comfortable merging it after confirming that the reset behavior is consistent with how this project already handles session-aware periods. |
|
Hi @robertotena , jumping in as another contributor. I think the reviewer's concern boils down to: A minimal way to inherit that session-awareness for free is to delegate to private bool IsNewQuarter(int bar)
{
if (!IsNewMonth(bar)) return false;
var month = GetCandle(bar).Time.Month;
return month == 1 || month == 4 || month == 7 || month == 10;
}
private bool IsNewYear(int bar)
{
if (!IsNewMonth(bar)) return false;
return GetCandle(bar).Time.Month == 1;
}Feel free to take it as-is, tweak it, or go a different way - just sharing in case it unblocks the PR. Happy to help further if you want. |
Added VWAP yearly and quarterly periods