ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/qlib
File: qlib/log.py (Python)

Code before:


class LogFilter(logging.Filter):
    def __init__(self, param=None):
        self.param = param

    @staticmethod
    def match_msg(filter_str, msg):
        match = False
        try:
            if re.match(filter_str, msg):
                match = True
        except Exception:
            pass
        return match

    def filter(self, record):
        allow = True
        if isinstance(self.param, str):
            allow = not self.match_msg(self.param, record.msg)
        elif isinstance(self.param, list):
            allow = not any([self.match_msg(p, record.msg) for p in self.param])
        return allow


def set_global_logger_level(level: int):
    """set qlib.xxx logger handlers level

    Parameters
    ----------
    level: int
        logger level

    Examples
    ---------

        .. code-block:: python

            import qlib
            import logging
            from qlib.log import get_module_logger, set_global_logger_level
            qlib.init()

            tmp_logger_01 = get_module_logger("tmp_logger_01", level=logging.INFO)
            tmp_logger_01.info("1. tmp_logger_01 info show")

            global_level = logging.WARNING + 1
            set_global_logger_level(global_level)
            tmp_logger_02 = get_module_logger("tmp_logger_02", level=logging.INFO)
            tmp_logger_02.log(msg="2. tmp_logger_02 log show", level=global_level)


Review comment (suggestion):
@zhupr  You can add a parameter like `return_orig_handler_level=False` to control the return value to reuse this function in  `set_global_logger_level_contextmanager`. 

Code after:


class LogFilter(logging.Filter):
    def __init__(self, param=None):
        self.param = param

    @staticmethod
    def match_msg(filter_str, msg):
        match = False
        try:
            if re.match(filter_str, msg):
                match = True
        except Exception:
            pass
        return match

    def filter(self, record):
        allow = True
        if isinstance(self.param, str):
            allow = not self.match_msg(self.param, record.msg)
        elif isinstance(self.param, list):
            allow = not any([self.match_msg(p, record.msg) for p in self.param])
        return allow


def set_global_logger_level(level: int, return_orig_handler_level: bool = False):
    """set qlib.xxx logger handlers level

    Parameters
    ----------
    level: int
        logger level

    return_orig_handler_level: bool
        return origin handler level map

    Examples
    ---------

        .. code-block:: python

            import qlib
            import logging
            from qlib.log import get_module_logger, set_global_logger_level
            qlib.init()

            tmp_logger_01 = get_module_logger("tmp_logger_01", level=logging.INFO)
            tmp_logger_01.info("1. tmp_logger_01 info show")

            global_level = logging.WARNING + 1
            set_global_logger_level(global_level)
Repository: microsoft/qlib
File: qlib/workflow/task/manage.py (Python)

Code before:
    The tasks manager assumes that you will only update the tasks you fetched.
    The mongo fetch one and update will make it date updating secure.

    .. note::

        Assumption: the data in MongoDB was encoded and the data out of MongoDB was decoded

    Here are four status which are:

        STATUS_WAITING: waiting for training

        STATUS_RUNNING: training

        STATUS_PART_DONE: finished some step and waiting for next step

        STATUS_DONE: all work done
    """

    STATUS_WAITING = "waiting"
    STATUS_RUNNING = "running"
    STATUS_DONE = "done"
    STATUS_PART_DONE = "part_done"

    ENCODE_FIELDS_PREFIX = ["def", "res"]

    def __init__(self, task_pool: str):
        """
        Init Task Manager, remember to make the statement of MongoDB url and database name firstly.

        Parameters
        ----------
        task_pool: str
            the name of Collection in MongoDB
        """
        self.mdb = get_mongodb()
        self.task_pool = getattr(self.mdb, task_pool)
        self.logger = get_module_logger(self.__class__.__name__)

    def list(self) -> list:
        """
        List the all collection(task_pool) of the db

        Returns:
            list
        """
        return self.mdb.list_collection_names()

    def _encode_task(self, task):
        for prefix in self.ENCODE_FIELDS_PREFIX:
            for k in list(task.keys()):
                if k.startswith(prefix):

Review comment (suggestion):
`task_pool` could be `None` if `TaskManger.list` is called.
Add docs about it in case that other contributors have similar confisions.

Code after:
    The tasks manager assumes that you will only update the tasks you fetched.
    The mongo fetch one and update will make it date updating secure.

    .. note::

        Assumption: the data in MongoDB was encoded and the data out of MongoDB was decoded

    Here are four status which are:

        STATUS_WAITING: waiting for training

        STATUS_RUNNING: training

        STATUS_PART_DONE: finished some step and waiting for next step

        STATUS_DONE: all work done
    """

    STATUS_WAITING = "waiting"
    STATUS_RUNNING = "running"
    STATUS_DONE = "done"
    STATUS_PART_DONE = "part_done"

    ENCODE_FIELDS_PREFIX = ["def", "res"]

    def __init__(self, task_pool: str):
        """
        Init Task Manager, remember to make the statement of MongoDB url and database name firstly.
        A TaskManager instance serves a specific task pool.
        The static method of this module serves the whole MongoDB.

        Parameters
        ----------
        task_pool: str
            the name of Collection in MongoDB
        """
        self.task_pool = getattr(get_mongodb(), task_pool)
        self.logger = get_module_logger(self.__class__.__name__)

    @staticmethod
    def list() -> list:
        """
        List the all collection(task_pool) of the db.

        Returns:
            list
        """
        return get_mongodb().list_collection_names()

    def _encode_task(self, task):
        for prefix in self.ENCODE_FIELDS_PREFIX:
Repository: microsoft/qlib
File: qlib/workflow/online/utils.py (Python)

Code before:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

"""
OnlineTool is a module to set and unset a series of `online` models.
The `online` models are some decisive models in some time points, which can be changed with the change of time.
This allows us to use efficient submodels as the market-style changing.
"""

from typing import List, Union
from qlib.data.dataset import TSDatasetH

from qlib.log import get_module_logger
from qlib.utils import get_cls_kwargs
from qlib.utils.exceptions import QlibException
from qlib.workflow.online.update import PredUpdater
from qlib.workflow.recorder import Recorder
from qlib.workflow.task.utils import list_recorders


class OnlineTool:
    """
    OnlineTool will manage `online` models in an experiment that includes the model recorders.
    """

    ONLINE_KEY = "online_status"  # the online status key in recorder
    ONLINE_TAG = "online"  # the 'online' model
    OFFLINE_TAG = "offline"  # the 'offline' model, not for online serving

    def __init__(self):
        """
        Init OnlineTool.
        """
        self.logger = get_module_logger(self.__class__.__name__)

    def set_online_tag(self, tag, recorder: Union[list, object]):
        """
        Set `tag` to the model to sign whether online.

        Args:

Review comment (suggestion):
`QlibException` looks too general
You can create a new exception class based on `QlibException` which is more concrete.

Code after:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

"""
OnlineTool is a module to set and unset a series of `online` models.
The `online` models are some decisive models in some time points, which can be changed with the change of time.
This allows us to use efficient submodels as the market-style changing.
"""

from typing import List, Union
from qlib.data.dataset import TSDatasetH

from qlib.log import get_module_logger
from qlib.utils import get_cls_kwargs
from qlib.utils.exceptions import LoadObjectError
from qlib.workflow.online.update import PredUpdater
from qlib.workflow.recorder import Recorder
from qlib.workflow.task.utils import list_recorders


class OnlineTool:
    """
    OnlineTool will manage `online` models in an experiment that includes the model recorders.
    """

    ONLINE_KEY = "online_status"  # the online status key in recorder
    ONLINE_TAG = "online"  # the 'online' model
    OFFLINE_TAG = "offline"  # the 'offline' model, not for online serving

    def __init__(self):
        """
        Init OnlineTool.
        """
        self.logger = get_module_logger(self.__class__.__name__)

    def set_online_tag(self, tag, recorder: Union[list, object]):
        """
        Set `tag` to the model to sign whether online.

        Args:
Repository: microsoft/qlib
File: qlib/utils/resam.py (Python)

Code before:
import numpy as np
import pandas as pd

from functools import partial
from typing import Union, Callable

from . import lazy_sort_index
from .time import Freq, cal_sam_minute


def resam_calendar(
    calendar_raw: np.ndarray, freq_raw: Union[str, Freq], freq_sam: Union[str, Freq], region: str
) -> np.ndarray:
    """
    Resample the calendar with frequency freq_raw into the calendar with frequency freq_sam
    Assumption:
        - Fix length (240) of the calendar in each day.

    Parameters
    ----------
    calendar_raw : np.ndarray
        The calendar with frequency  freq_raw
    freq_raw : str
        Frequency of the raw calendar
    freq_sam : str
        Sample frequency
    region: str
        Region, for example, "cn", "us"
    Returns
    -------
    np.ndarray
        The calendar with frequency freq_sam
    """
    freq_raw = Freq(freq_raw)
    freq_sam = Freq(freq_sam)
    if not len(calendar_raw):
        return calendar_raw

Review comment (suggestion):
I think we can make the region an optional parameter.
And the default value is C['region'] if it is None

Otherwise, all the places need explicitly values when call this function.
![image](https://user-images.githubusercontent.com/465606/163558732-a4032f90-0cf8-4d9e-904f-c43da2e55b3b.png)


Code after:
import numpy as np
import pandas as pd

from functools import partial
from typing import Union, Callable

from . import lazy_sort_index
from .time import Freq, cal_sam_minute
from ..config import C


def resam_calendar(
    calendar_raw: np.ndarray, freq_raw: Union[str, Freq], freq_sam: Union[str, Freq], region: str = None
) -> np.ndarray:
    """
    Resample the calendar with frequency freq_raw into the calendar with frequency freq_sam
    Assumption:
        - Fix length (240) of the calendar in each day.

    Parameters
    ----------
    calendar_raw : np.ndarray
        The calendar with frequency  freq_raw
    freq_raw : str
        Frequency of the raw calendar
    freq_sam : str
        Sample frequency
    region: str
        Region, for example, "cn", "us"
    Returns
    -------
    np.ndarray
        The calendar with frequency freq_sam
    """
    if region is None:
        region = C["region"]
Repository: microsoft/qlib
File: qlib/data/ops.py (Python)

Code before:

    Parameters
    ----------
    feature : Expression
        feature instance
    N : int
        rolling window size

    Returns
    ----------
    Expression
        a feature instance with rolling rank
    """

    def __init__(self, feature, N):
        super(Rank, self).__init__(feature, N, "rank")

    def _load_internal(self, instrument, start_index, end_index, *args):
        series = self.feature.load(instrument, start_index, end_index, *args)
        # TODO: implement in Cython

        def rank(x):
            if np.isnan(x[-1]):
                return np.nan
            x1 = x[~np.isnan(x)]
            if x1.shape[0] == 0:
                return np.nan
            return percentileofscore(x1, x1[-1]) / 100

        if self.N == 0:
            series = series.expanding(min_periods=1).apply(rank, raw=True)
        else:
            series = series.rolling(self.N, min_periods=1).apply(rank, raw=True)
        return series


class Count(Rolling):
    """Rolling Count

    Parameters
    ----------
    feature : Expression
        feature instance
    N : int
        rolling window size

    Returns
    ----------
    Expression
        a feature instance with rolling count of number of non-NaN elements
    """

Review comment (suggestion):
I think we can make it simpler by using pandas' native API like this 

```
series.rolling(N, min_periods=1).rank(pct=True)
```

Code after:
        rolling window size

    Returns
    ----------
    Expression
        a feature instance with rolling rank
    """

    def __init__(self, feature, N):
        super(Rank, self).__init__(feature, N, "rank")

    # for compatiblity of python 3.7, which doesn't support pandas 1.4.0+ which implements Rolling.rank
    def _load_internal(self, instrument, start_index, end_index, *args):
        series = self.feature.load(instrument, start_index, end_index, *args)

        rolling_or_expending = series.expanding(min_periods=1) if self.N == 0 else series.rolling(self.N, min_periods=1)
        if hasattr(rolling_or_expending, "rank"):
            return rolling_or_expending.rank(pct=True)

        def rank(x):
            if np.isnan(x[-1]):
                return np.nan
            x1 = x[~np.isnan(x)]
            if x1.shape[0] == 0:
                return np.nan
            return percentileofscore(x1, x1[-1]) / 100

        return rolling_or_expending.apply(rank, raw=True)


class Count(Rolling):
    """Rolling Count

    Parameters
    ----------
    feature : Expression
        feature instance
    N : int
        rolling window size

    Returns
    ----------
    Expression
        a feature instance with rolling count of number of non-NaN elements
    """

    def __init__(self, feature, N):
        super(Count, self).__init__(feature, N, "count")


class Delta(Rolling):
Repository: microsoft/qlib
File: qlib/rl/order_execution/reward.py (Python)

Code before:
        Maximum number of steps.
    start_time_index
        First time index that allowed to trade.
    end_time_index
        Last time index that allowed to trade.
    """
    
    def __init__(self, max_step: int, start_time_index: int = 0, end_time_index: int = 239) -> None:
        self.max_step = max_step
        self.start_time_index = start_time_index
        self.end_time_index = end_time_index
    
    def reward(self, simulator_state: SAOEState) -> float:
        if simulator_state.cur_step == self.max_step - 1 or simulator_state.position < 1e-6:
            traded = simulator_state.order.amount - simulator_state.position
            trade_value = sum(simulator_state.history_steps["trade_value"])
            intraday_data = simulator_state.backtest_data.data[self.start_time_index:self.end_time_index + 1]
            
            if traded >= 1e-6:
                vwap_price = trade_value / traded
            else:
                vwap_price = _weighted_average(  # VWAP price of the entire day
                    val=intraday_data["$close0"].to_numpy(), 
                    weight=intraday_data["$volume0"].to_numpy(),
                )
            twap_price = np.mean(intraday_data["$close0"].to_numpy())
            
            ratio = vwap_price / twap_price if simulator_state.order.direction == OrderDir.SELL else twap_price / vwap_price
            if ratio < 1.0:
                return -1.0
            elif ratio < 1.1:
                return 0.0
            else:
                return 1.0
        else:
            return 0.0


Review comment (suggestion):
We should use actual deal price instead of $close0. Might use
```
vwap_price = simulator_state.metrics.trade_price
twap_price = simulator_state.backtest_data.get_deal_price().mean()
```

Code after:
        Last time index that allowed to trade.
    """

    def __init__(self, max_step: int, start_time_index: int = 0, end_time_index: int = 239) -> None:
        self.max_step = max_step
        self.start_time_index = start_time_index
        self.end_time_index = end_time_index

    def reward(self, simulator_state: SAOEState) -> float:
        if simulator_state.cur_step == self.max_step - 1 or simulator_state.position < 1e-6:
            vwap_price = cast(dict, simulator_state.metrics)["trade_price"]
            twap_price = simulator_state.backtest_data.get_deal_price().mean()

            if simulator_state.order.direction == OrderDir.SELL:
                ratio = vwap_price / twap_price if twap_price != 0 else 1.0
            else:
                ratio = twap_price / vwap_price if vwap_price != 0 else 1.0
            if ratio < 1.0:
                return -1.0
            elif ratio < 1.1:
                return 0.0
            else:
                return 1.0
        else:
            return 0.0
Repository: microsoft/qlib
File: qlib/finco/test_sumarize.py (Python)

Code before:
import unittest

from qlib.finco.task import SummarizeTask


class TestSummarize(unittest.TestCase):
    def test_parse2txt(self):
        task = SummarizeTask()
        resp = task.parse2txt('')
        print(resp)


if __name__ == '__main__':
    unittest.main()


Review comment (suggestion):
```suggestion
from dotenv import load_dotenv
# pydantic support load_dotenv,   so load_dotenv will be deprecated in the future.
```

Code after:
import unittest
from dotenv import load_dotenv

from qlib.finco.task import SummarizeTask
from qlib.finco.workflow import WorkflowContextManager

load_dotenv(verbose=True, override=True)


class TestSummarize(unittest.TestCase):

    def test_execution(self):
        task = SummarizeTask()
        task.assign_context_manager(WorkflowContextManager())
        resp = task.execution()
        print(resp)

    def test_parse2txt(self):
        task = SummarizeTask()
        resp = task.parse2txt('')
        print(resp)


if __name__ == '__main__':
    unittest.main()
Repository: microsoft/qlib
File: qlib/contrib/strategy/cost_control.py (Python)

Code before:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

from .order_generator import OrderGenWInteract
from .signal_strategy import WeightStrategyBase


class SoftTopkStrategy(WeightStrategyBase):
    def __init__(
        self,
        model=None,
        dataset=None,
        topk=None,
        order_generator_cls_or_obj=OrderGenWInteract,
        max_sold_weight=1.0,
        trade_impact_limit=None,
        priority="IMPACT_FIRST",
        risk_degree=0.95,
        buy_method="first_fill",
        **kwargs,
    ):
        """
        Refactored SoftTopkStrategy with a budget-constrained rebalancing engine.

        Parameters
        ----------
        topk : int
            The number of top-N stocks to be held in the portfolio.
        trade_impact_limit : float
            Maximum weight change for each stock in one trade.
        priority : str
            "COMPLIANCE_FIRST" or "IMPACT_FIRST".
        risk_degree : float
            The target percentage of total value to be invested.
        """
        super(SoftTopkStrategy, self).__init__(
            model=model, dataset=dataset, order_generator_cls_or_obj=order_generator_cls_or_obj, **kwargs
        )

        self.topk = topk
        self.trade_impact_limit = trade_impact_limit if trade_impact_limit is not None else max_sold_weight
        self.priority = priority.upper()

Review comment (suggestion):
I think we don't have to define an extra parameter.
Setting max_sold_weight=1.0 indicates `COMPLIANCE_FIRST`

Code after:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

from .order_generator import OrderGenWInteract
from .signal_strategy import WeightStrategyBase


class SoftTopkStrategy(WeightStrategyBase):
    def __init__(
        self,
        model=None,
        dataset=None,
        topk=None,
        order_generator_cls_or_obj=OrderGenWInteract,
        max_sold_weight=1.0,
        trade_impact_limit=None,
        risk_degree=0.95,
        buy_method="first_fill",
        **kwargs,
    ):
        """
        Refactored SoftTopkStrategy with a budget-constrained rebalancing engine.

        Parameters
        ----------
        topk : int
            The number of top-N stocks to be held in the portfolio.
        trade_impact_limit : float
            Maximum weight change for each stock in one trade. If None, fallback to max_sold_weight.
        max_sold_weight : float
            Backward-compatible alias for trade_impact_limit. Use 1.0 to effectively disable the limit.
        risk_degree : float
            The target percentage of total value to be invested.
        """
        super(SoftTopkStrategy, self).__init__(
            model=model, dataset=dataset, order_generator_cls_or_obj=order_generator_cls_or_obj, **kwargs
        )

        self.topk = topk
        self.trade_impact_limit = trade_impact_limit if trade_impact_limit is not None else max_sold_weight
        self.risk_degree = risk_degree
        self.buy_method = buy_method
Repository: microsoft/semantic-kernel
File: dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs (C#)

Code before:
﻿// Copyright (c) Microsoft. All rights reserved.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.IO;
using System.Text;
using System.Threading.Tasks;

namespace Microsoft.SemanticKernel.Plugins.Core;

/// <summary>
/// Read and write from a file.
/// </summary>
public sealed class FileIOPlugin
{
    /// <summary>
    /// List of allowed folders to download to.
    /// </summary>
    public IEnumerable<string>? AllowedFolders
    {
        get => this._allowedFolders;
        set => this._allowedFolders = value is null ? null : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
    }

    /// <summary>
    /// Set to true to disable overwriting existing files.
    /// </summary>
    public bool DisableFileOverwrite { get; set; } = false;

    /// <summary>
    /// Read a file
    /// </summary>
    /// <example>
    /// {{file.readAsync $path }} => "hello world"
    /// </example>
    /// <param name="path"> Source file </param>
    /// <returns> File content </returns>
    [KernelFunction, Description("Read a file")]
    public async Task<string> ReadAsync([Description("Source file")] string path)
    {
        if (!this.IsFilePathAllowed(path))
        {

Review comment (suggestion):
```suggestion
    /// List of allowed folders to read from or write to.
```

Code after:
﻿// Copyright (c) Microsoft. All rights reserved.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.IO;
using System.Text;
using System.Threading.Tasks;

namespace Microsoft.SemanticKernel.Plugins.Core;

/// <summary>
/// Read and write from a file.
/// </summary>
public sealed class FileIOPlugin
{
    /// <summary>
    /// List of allowed folders to read from or write to.
    /// </summary>
    public IEnumerable<string>? AllowedFolders
    {
        get => this._allowedFolders;
        set => this._allowedFolders = value is null ? null : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
    }

    /// <summary>
    /// Set to true to disable overwriting existing files.
    /// </summary>
    public bool DisableFileOverwrite { get; set; } = false;

    /// <summary>
    /// Read a file
    /// </summary>
    /// <example>
    /// {{file.readAsync $path }} => "hello world"
    /// </example>
    /// <param name="path"> Source file </param>
    /// <returns> File content </returns>
    [KernelFunction, Description("Read a file")]
    public async Task<string> ReadAsync([Description("Source file")] string path)
    {
        if (!this.IsFilePathAllowed(path))
        {