Skip to content

feat: implement VWAP factor (#18)#23

Open
hari2k7 wants to merge 2 commits intoNeuZhou:masterfrom
hari2k7:feature/vwap-factor
Open

feat: implement VWAP factor (#18)#23
hari2k7 wants to merge 2 commits intoNeuZhou:masterfrom
hari2k7:feature/vwap-factor

Conversation

@hari2k7
Copy link
Copy Markdown

@hari2k7 hari2k7 commented Apr 12, 2026

Summary

Implements a rolling VWAP (Volume Weighted Average Price) factor that
measures deviation of close price from VWAP over a rolling window.

Changes

  • Add factors/vwap.py with VWAPFactor class
  • Add tests/test_vwap.py with 7 tests

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 📝 Documentation update
  • 🧪 Test improvement
  • ♻️ Refactor (no functional change)

Testing

  • All existing tests pass (pytest)
  • New tests added for new functionality
  • Linting passes (ruff check src/)
  • Manually tested with relevant commands

Checklist

  • Code follows the project's style guidelines
  • Self-review of own code completed
  • Documentation updated (if applicable)
  • No new warnings introduced

Related Issues

Closes #18

Screenshots / Demo

Terminal output

$ pytest tests/test_vwap.py -v

tests/test_vwap.py::TestVWAPFactor::test_output_length_matches_input PASSED
tests/test_vwap.py::TestVWAPFactor::test_nan_before_period PASSED
tests/test_vwap.py::TestVWAPFactor::test_finite_after_period PASSED
tests/test_vwap.py::TestVWAPFactor::test_zero_volume_edge_case PASSED
tests/test_vwap.py::TestVWAPFactor::test_known_value PASSED
tests/test_vwap.py::TestVWAPFactor::test_default_params PASSED
tests/test_vwap.py::TestVWAPFactor::test_param_ranges PASSED

7 passed in 0.06s

output

Copy link
Copy Markdown
Owner

@NeuZhou NeuZhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, but there are structural issues that need fixing before merge:

🔴 Critical: Wrong file location

The factor is placed at \ actors/vwap.py\ (repo root) instead of \stratevo/factors/builtin/volume.py\ or similar path under the package. This means:

  • It won't be auto-discovered by the factor registry
  • The test imports from \ actors.vwap\ which only works with the sys.path hack, not as a proper package import
  • Other volume factors live in \stratevo/factors/builtin/\ — this should follow the same pattern

Fix: Move to \stratevo/factors/builtin/volume.py\ (create it or append to existing) and register in the volume category. Update test imports accordingly.

🟡 Performance: O(n × period) nested loop

The current implementation uses a nested Python for-loop which will be very slow on large datasets (>1000 bars with period=100 → 100k iterations). Consider using numpy cumsum for O(n):
\\python
cum_pv = np.cumsum(closes * volumes)
cum_v = np.cumsum(volumes)

rolling sum via difference

\\

🟡 Missing newline at EOF

Both files are missing trailing newlines (\No newline at end of file).

✅ What's good

  • Clean API matching BaseFactor interface
  • Good test coverage (7 tests including edge cases)
  • Zero-volume handling is correct
  • Known-value test with manual calculation is great

@NeuZhou
Copy link
Copy Markdown
Owner

NeuZhou commented Apr 13, 2026

Hey @hari2k7, thanks for this contribution! 🙏 Just checking in — any updates on the requested changes (moving the file to the correct location)? Let me know if you need any help with the restructuring, happy to guide you through it!

@hari2k7
Copy link
Copy Markdown
Author

hari2k7 commented Apr 13, 2026

Fixed all review issues:

  • Moved VWAPFactor to stratevo/factors/builtin/volume.py
  • Added compute_vwap() to indicators_builtin.py
  • Updated test imports to use proper package path
  • Removed old factors/vwap.py
  • Added test for None volumes case (8 tests total)

All 8 tests passing ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Factor: Implement VWAP (Volume Weighted Average Price) factor

2 participants