-
Notifications
You must be signed in to change notification settings - Fork 0
Halvix code review proposal #4
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
Conversation
Co-authored-by: pierre.juillard <[email protected]>
This commit introduces the new TOTAL2b index calculation methodology, which offers a simpler and more predictable approach to index composition. The documentation has been significantly updated to detail both TOTAL2 and TOTAL2b, including their differences, configuration, and calculation algorithms.
Key changes include:
- **Introduction of TOTAL2b:** A new calculation method with a freeze period for new coins and price scaling instead of iterative capping.
- **Documentation Overhaul:** Comprehensive updates to `docs/TOTAL2_CALCULATION.md` explaining both methodologies, their parameters, and nuances.
- **Code Refactoring:**
- Separation of base processing logic into `processor_base.py`.
- Dedicated modules for `processor_total2.py` (legacy) and `processor_total2b.py` (new).
- Introduction of a factory function `get_processor` in `data/__init__.py` and `data/processor.py` for selecting the appropriate processor.
- **Testing Enhancements:** Added tests for the new `Total2bProcessor` and the `get_processor` factory function.
- **Configuration Updates:** Added new configuration parameters specific to TOTAL2b (e.g., `FREEZE_PERIOD_DAYS`, `MIN_COINS_FOR_SCALING`).
- **CLI Option:** Added `--index-type` option to `main.py` to select between `total2` and `total2b`.
This refactoring aims to improve code clarity, maintainability, and provide users with a more robust and understandable index calculation.
Co-authored-by: pierre.juillard <[email protected]>
|
Cursor Agent can help with this pull request. Just |
Co-authored-by: pierre.juillard <[email protected]>
Co-authored-by: pierre.juillard <[email protected]>
Co-authored-by: pierre.juillard <[email protected]>
Co-authored-by: pierre.juillard <[email protected]>
Co-authored-by: pierre.juillard <[email protected]>
Co-authored-by: pierre.juillard <[email protected]>
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 is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 11
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ) | ||
|
|
||
| # Update coins in index | ||
| coins_in_index = set(eligible_coins) |
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.
Bug: TOTAL2b tracks all eligible coins instead of TOP-N
The coins_in_index variable is updated with all eligible coins (set(eligible_coins)) instead of only the TOP-N coins that are actually in the index. According to the documentation, price scaling should be applied when "coins entering TOP30 today", but the current implementation detects coins when they become eligible (pass freeze period), not when they actually enter the TOP-N. This means a coin that becomes eligible but isn't in TOP-N won't get scaled when it later enters the index. The fix would be to update coins_in_index after the top_n calculation, using only the coin IDs from top_n.
Additional Locations (1)
| "change_factor": float(price_change), | ||
| "days_since_entry": days_since_entry, | ||
| } | ||
| ) |
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.
Bug: Entry warmup corrections recorded but never applied
The _apply_entry_warmup_corrections method's docstring states it modifies total2_series in place, but the function never actually modifies the series. It only records correction events in a list. The comment on line 402-403 acknowledges this is "(simplified - full implementation would recalculate weighted avg)". This is a regression from the original implementation which actually applied price capping to coin prices and recalculated TOTAL2. The documented entry warmup behavior (capping price changes at ±70%/50% for 21 days after entry) is not being enforced.
Implements the new TOTAL2b index and refactors the index calculation codebase.
Addresses GitHub issue #2 by introducing a new index methodology with a freeze period and price scaling, and improves code modularity by separating concerns into base and specific processor classes. Backward compatibility was explicitly not a requirement for this change.
Note
Introduces the TOTAL2b index (21‑day freeze + price scaling), refactors TOTAL2 processing into base/legacy/new processors with a factory, adds a CLI
--index-type(defaulttotal2b), and updates docs/tests accordingly.Total2bProcessor(src/data/processor_total2b.py) with 21‑day freeze period and price scaling on entry; no price outlier correction.BaseTotal2Processor(src/data/processor_base.py).Total2Processor(src/data/processor_total2.py) with iterative price capping/outlier handling.get_processorand constants re‑exports insrc/data/processor.py; updatesrc/data/__init__.pyexports.calculate-total2to useget_processorand support--index-type {total2|total2b}(defaulttotal2b).tests/test_processor.pyto cover factory, TOTAL2b freeze/scaling, shared behavior, and edge cases.docs/TOTAL2_CALCULATION.md).AGENTS.md; updateREADME.mdreferences; removedocs/PROJECT_CONTEXT.md..cursorrules.Written by Cursor Bugbot for commit 0b5e736. This will update automatically on new commits. Configure here.