Merge pull request #119 from cvxgrp/improve_docs_execution_model

Improve base methods signatures

.github/workflows/test.yml

@@ -87,6 +87,7 @@ jobs:
               coverage lcov
 
       - name: Coveralls GitHub Action
+        if: ${{ github.event_name == 'push'}}
         uses: coverallsapp/github-action@v1
         with:
           path-to-lcov: coverage.lcov

Makefile

@@ -37,7 +37,7 @@ test:  ## run tests w/ cov report
 	# $(BINDIR)/bandit $(PROJECT)/*.py $(TESTS)/*.py
 
 lint:  ## run linter
-	$(BINDIR)/pylint $(PROJECT) $(EXTRA_SCRIPTS) $(EXAMPLES)
+	$(BINDIR)/pylint $(PROJECT) $(EXTRA_SCRIPTS) # $(EXAMPLES)
 	$(BINDIR)/diff-quality --violations=pylint --config-file pyproject.toml
 
 docs:  ## build docs

TODOs_ROADMAP.rst

@@ -52,15 +52,29 @@ their planned release.
 
   - [ ] ``initialize_estimator`` could get optional market data partial
     signature for caching. Default None, no incompatible change.
-  - [ ] Could get a ``finalize_estimator`` method used for storing
+  - [X] Could get a ``finalize_estimator`` method used for storing
     data, like risk models on disk, doesn't need arguments; it can use the
     partial signature got above. No incompatible change.
 
 ``cvxportfolio.data``
 --------------------------
 
+- [ ] Improve ``YahooFinance`` data cleaning. Idea is to factor it in a 
+  base ``OpenLowHighCloseVolume`` class, which should flexible enough to
+  accommodate adjusted closes (i.e., with backwards dividend adjustments like
+  YF), total returns like other data sources, or neither for non-stocks assets.
+  This would implement all data cleaning process as sequence of small steps
+  in separate methods, with good logging. It would also implement data quality
+  check in the ``preload`` method to give feedback to the user. PR #125
+- [ ] Factor ``data.py`` in ``data/`` submodule. PR #125
+
 ``cvxportfolio.simulator``
 --------------------------
+- [ ] Make ``BackTestResult`` interface methods with ``MarketSimulator`` 
+  public. It probably should do a context manager b/c logging code in 
+  ``BackTestResult`` does cleanup of loggers at the end, to ensure all right
+  in case back-test fails. 
+- [ ] Move caching logic out of it; see above.
 
 ``cvxportfolio.risks``
 ----------------------
@@ -95,7 +109,7 @@ Optimization policies
   compatibility, it doesn't if we don't give defaults (so exceptions are raised
   all the way to the caller), but then it's extra complication (more 
   arguments). Consider for ``2.0.0``.
-- [ ] Improve ``__repr__`` method, now hard to read. Target ``1.1.1``.
+- [X] Improve ``__repr__`` method, now hard to read. Target ``1.1.0``.
 
 ``cvxportfolio.constraints``
 ----------------------------
@@ -106,8 +120,6 @@ Optimization policies
 ``cvxportfolio.result``
 -----------------------
 
-- [ ] Make ``BackTestResult`` interface methods with ``MarketSimulator`` 
-  public. 
 - [ ] Add a ``bankruptcy`` property (boolean). Amend ``sharpe_ratio``
   and other aggregate statistics (as best as possible) to return ``-np.inf``
   if back-test ended in backruptcy. This is needed specifically for
@@ -122,7 +134,7 @@ Optimization policies
 Other 
 -----
 
-- [ ] Exceptions are not too good, probably ``cvxportfolio.DataError`` should
+- [X] Exceptions are not too good, probably ``cvxportfolio.DataError`` should
   be ``ValueError``, .... Research this, one option is to simply derive from
   built-ins (``class DataError(ValueError): pass``), .... No compatibility
   breaks.
@@ -140,8 +152,8 @@ Documentation
 -------------
 
 - [ ] Improve examples section, also how "Hello world" is mentioned in readme.
-- [ ] Manual.
-- [ ] Quickstart, probably to merge into manual.
+- [ ] Manual. PR #124
+- [ ] Quickstart, probably to merge into manual. PR #124
 
 Examples
 --------

bumpversion.py

@@ -34,7 +34,7 @@ def findversion(root='.'):
     We use the first __init__.py with a __version__ that we find.
 
     :param root: Root folder of the project.
-    :type root: Pathlib.Path or str
+    :type root: pathlib.Path or str
 
     :raises ValueError: No version found.
 
@@ -64,6 +64,13 @@ def replaceversion(new_version, version, root='.'):
     """Replace version number. Skip [env, venv, .*].
 
     We replace in all __init__.py, conf.py, setup.py, and pyproject.toml
+
+    :param new_version: New version.
+    :type new_version: str
+    :param version: Old version.
+    :type version: str
+    :param root: Root folder of the project.
+    :type root: pathlib.Path or str
     """
 
     p = Path(root)
@@ -73,14 +80,14 @@ def replaceversion(new_version, version, root='.'):
             'pyproject.toml']:
 
             lines = []
-            with open(fname, 'rt') as fin:
+            with open(fname, 'rt', encoding="utf-8") as fin:
                 for line in fin:
                     lines.append(line.replace(version, new_version))
 
-            with open(fname, "wt") as fout:
+            with open(fname, "wt", encoding="utf-8") as fout:
                 for line in lines:
                     fout.write(line)
-            subprocess.run(['git', 'add', str(fname)])
+            subprocess.run(['git', 'add', str(fname)], check=False)
 
         if fname.is_dir():
             if not (fname.name in ['env', 'venv'] or fname.name[0] == '.'):
@@ -91,28 +98,29 @@ def replaceversion(new_version, version, root='.'):
 
     while True:
         print('[revision/minor/major]')
-        what = input()
-        if what in ['revision', 'minor', 'major']:
+        WHAT = input()
+        if WHAT in ['revision', 'minor', 'major']:
             break
 
-    version = findversion()
+    VERSION = findversion()
 
-    x, y, z = [int(el) for el in version.split('.')]
-    if what == 'revision':
-        z += 1
-    if what == 'minor':
-        y += 1
-        z = 0
-    if what == 'major':
-        x += 1
-        y = 0
-        z = 0
-    new_version = f"{x}.{y}.{z}"
+    X, Y, Z = [int(el) for el in VERSION.split('.')]
+    if WHAT == 'revision':
+        Z += 1
+    if WHAT == 'minor':
+        Y += 1
+        Z = 0
+    if WHAT == 'major':
+        X += 1
+        Y = 0
+        Z = 0
+    NEW_VERSION = f"{X}.{Y}.{Z}"
 
-    print(new_version)
+    print(NEW_VERSION)
 
-    replaceversion(new_version, version)
+    replaceversion(NEW_VERSION, VERSION)
     subprocess.run(['git', 'commit', '--no-verify', '-em',
-        f"version {new_version}\n"])
-    subprocess.run(['git', 'tag', new_version])
-    subprocess.run(['git', 'push', '--no-verify', 'origin', new_version])
+        f"version {NEW_VERSION}\n"], check=False)
+    subprocess.run(['git', 'tag', NEW_VERSION], check=False)
+    subprocess.run(
+        ['git', 'push', '--no-verify', 'origin', NEW_VERSION], check=False)

cvxportfolio/constraints.py

@@ -233,17 +233,19 @@ def __init__(self, window=250, #benchmark=MarketBenchmark
         # self.benchmark = benchmark
         self.market_vector = None
 
-    def initialize_estimator(self, universe, trading_calendar):
+    def initialize_estimator( # pylint: disable=arguments-differ
+            self, universe, **kwargs):
         """Initialize parameter with size of universe.
 
         :param universe: Trading universe, including cash.
         :type universe: pandas.Index
-        :param trading_calendar: Future (including current) trading calendar.
-        :type trading_calendar: pandas.DatetimeIndex
+        :param kwargs: Other unused arguments to :meth:`initialize_estimator`.
+        :type kwargs: dict
         """
         self.market_vector = cp.Parameter(len(universe)-1)
 
-    def values_in_time(self, past_volumes, **kwargs):
+    def values_in_time( # pylint: disable=arguments-differ
+            self, past_volumes, **kwargs):
         """Update parameter with current market weights and covariance.
 
         :param past_volumes: Past market volumes, in units of value.
@@ -315,7 +317,8 @@ def __init__(self, volumes, max_fraction_of_volumes=0.05):
             max_fraction_of_volumes, compile_parameter=True)
         self.portfolio_value = cp.Parameter(nonneg=True)
 
-    def values_in_time(self, current_portfolio_value, **kwargs):
+    def values_in_time( # pylint: disable=arguments-differ
+            self, current_portfolio_value, **kwargs):
         """Update parameter with current portfolio value.
 
         :param current_portfolio_value: Current total value of the portfolio.
@@ -385,20 +388,22 @@ def __init__(self, asset, periods):
         self._low = None
         self._high = None
 
-    def initialize_estimator(self, universe, trading_calendar):
+    def initialize_estimator( # pylint: disable=arguments-differ
+            self, universe, **kwargs):
         """Initialize internal parameters.
 
         :param universe: Trading universe, including cash.
         :type universe: pandas.Index
-        :param trading_calendar: Future (including current) trading calendar.
-        :type trading_calendar: pandas.DatetimeIndex
+        :param kwargs: Other unused arguments to :meth:`initialize_estimator`.
+        :type kwargs: dict
         """
         self._index = (universe.get_loc if hasattr(
             universe, 'get_loc') else universe.index)(self.asset)
         self._low = cp.Parameter()
         self._high = cp.Parameter()
 
-    def values_in_time(self, t, **kwargs):
+    def values_in_time( # pylint: disable=arguments-differ
+            self, t, **kwargs):
         """Update parameters, if necessary by imposing no-trade.
 
         :param t: Current time.
@@ -481,7 +486,8 @@ def __init__(self, c_min):
         self.c_min = DataEstimator(c_min)
         self.rhs = cp.Parameter()
 
-    def values_in_time(self, current_portfolio_value, **kwargs):
+    def values_in_time( # pylint: disable=arguments-differ
+            self, current_portfolio_value, **kwargs):
         """Update parameter with current portfolio value.
 
         :param current_portfolio_value: Current total value of the portfolio.
@@ -712,18 +718,20 @@ def __init__(self, limit, times):
         self.limit = None
         self.trading_calendar = None
 
-    def initialize_estimator(self, universe, trading_calendar):
+    def initialize_estimator( # pylint: disable=arguments-differ
+            self, trading_calendar, **kwargs):
         """Initialize estimator instance with updated trading_calendar.
 
-        :param universe: Trading universe, including cash.
-        :type universe: pandas.Index
         :param trading_calendar: Future (including current) trading calendar.
         :type trading_calendar: pandas.DatetimeIndex
+        :param kwargs: Other unused arguments to :meth:`initialize_estimator`.
+        :type kwargs: dict
         """
         self.trading_calendar = trading_calendar
         self.limit = cp.Parameter()
 
-    def values_in_time(self, t, mpo_step, **kwargs):
+    def values_in_time( # pylint: disable=arguments-differ
+            self, t, mpo_step, **kwargs):
         """If target period is in sight activate constraint.
 
         :param t: Current time.