From fac58562779fc6d6ee0646fcbbcd3ccb731bb603 Mon Sep 17 00:00:00 2001 From: guitaristsam Date: Fri, 1 May 2026 01:22:44 +0530 Subject: [PATCH] Document `step()` action-scaling contract; add `_validate_action_shares` hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `StockTradingEnv.step()` rescales the input `actions` from [-1, 1] to integer share counts via `actions = actions * self.hmax; actions.astype(int)` before any `_buy_stock` / `_sell_stock` call sees it. This is correct, but subclasses overriding `step()` to add custom action constraints commonly make one of two mistakes: 1. Validate / clip / round the raw [-1, 1] action BEFORE the parent's `* self.hmax` rescaling. Net effect: the action grid collapses to {-hmax, 0, +hmax} with no fine-grained levels in between, and the policy `std` gets stuck during PPO training because there's no gradient benefit to outputting fractional values. 2. Re-implement the entire `step()` flow to inject custom logic, which means the subclass has to track FinRL's internal state-layout conventions (`state[1:1+stock_dim]` is prices, not shares — a separate trap that this PR does not address but is mentioned as related). This PR adds two non-breaking changes: - A docstring on `step()` explicitly stating the [-1, 1] -> integer-shares rescaling contract and pointing subclassers at the new hook. - A `_validate_action_shares(action_shares)` method that runs after the scaling and turbulence override but before the buy/sell loop. Default implementation is identity, so existing subclasses are unaffected. Subclasses that want post-rescaling constraints (per-step share-delta caps, integer-only enforcement, tighter budget pre-checks, no-shorting) can now override this single method instead of re-implementing `step()`. Note on FinRL-X: I'm aware the project is moving toward FinRL-X, which uses a weight-centric interface and has no Gym env at all. This PR targets legacy FinRL specifically because the action-scaling trap is an issue for users following the existing educational/research tutorials, which explicitly remain on this repo. Real-world reproducer: I hit this trap while subclassing StockTradingEnv to enforce integer-share trading on Indian equities. v6 of my project silently collapsed the policy's action grid; the smoking gun was the PPO `std` parameter stuck at 0.97 across all 200k training timesteps. Tracking it down required reading line 314 of env_stocktrading.py and the offending subclass side-by-side. Full debugging diary at https://github.com/guitaristsam/RL_PPO_NIFTY50 (see the "Action scaling" section of CLAUDE.md). --- .../env_stock_trading/env_stocktrading.py | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/finrl/meta/env_stock_trading/env_stocktrading.py b/finrl/meta/env_stock_trading/env_stocktrading.py index 2fb3a69c84..81d6c431d7 100644 --- a/finrl/meta/env_stock_trading/env_stocktrading.py +++ b/finrl/meta/env_stock_trading/env_stocktrading.py @@ -223,12 +223,50 @@ def _do_buy(): return buy_num_shares + def _validate_action_shares(self, action_shares): + """Hook for subclasses to apply additional constraints on the integer + share count BEFORE ``_buy_stock`` / ``_sell_stock`` execution. + + Default implementation is identity. Override in a subclass to enforce + e.g. per-step share-delta caps, tighter budget clamps, no-shorting, + or any constraint that needs to operate on the post-``* self.hmax`` + share count. This avoids the common pitfall of overriding ``step`` + and accidentally validating the raw ``[-1, 1]`` action before the + parent's ``* self.hmax`` rescaling. + + Args: + action_shares: integer numpy array of shape ``(stock_dim,)`` with + the candidate per-stock share-delta (positive = buy, negative + = sell), as produced by ``actions * self.hmax``. + + Returns: + A modified integer numpy array of the same shape. + """ + return action_shares + def _make_plot(self): plt.plot(self.asset_memory, "r") plt.savefig(f"results/account_value_trade_{self.episode}.png") plt.close() def step(self, actions): + """Advance the environment by one bar. + + ``actions`` is the policy output, expected to be continuous and + normalised to ``[-1, 1]`` (this is the action_space defined in + ``__init__``). Inside this method the action is rescaled to an + integer share count via ``actions = actions * self.hmax`` followed + by ``astype(int)`` before any ``_sell_stock`` / ``_buy_stock`` call + sees it. + + Subclasses overriding ``step`` should validate the action AFTER the + ``* self.hmax`` rescaling, not before — applying an integer cast or + budget clamp to the raw ``[-1, 1]`` action will collapse the entire + action grid (e.g. clipping to ``{-1, 0, 1}`` pre-rescaling becomes + ``{-hmax, 0, hmax}`` post-rescaling, with no fine-grained levels in + between). For tighter post-rescaling constraints, override + ``_validate_action_shares`` rather than re-implementing this method. + """ self.terminal = self.day >= len(self.df.index.unique()) - 1 if self.terminal: # print(f"Episode: {self.episode}") @@ -318,6 +356,11 @@ def step(self, actions): if self.turbulence_threshold is not None: if self.turbulence >= self.turbulence_threshold: actions = np.array([-self.hmax] * self.stock_dim) + # Hook for subclasses that want to enforce additional constraints + # (per-step share-delta caps, tighter budget pre-checks, no-shorting + # rules, etc.) on the integer share count BEFORE buy/sell execution. + # Default implementation is identity; see _validate_action_shares. + actions = self._validate_action_shares(actions) begin_total_asset = self.state[0] + sum( np.array(self.state[1 : (self.stock_dim + 1)]) * np.array(self.state[(self.stock_dim + 1) : (self.stock_dim * 2 + 1)])