ronantakizawa/codereview-bench huggingface dataset:
Repository: quantopian/zipline
File: zipline/testing/predicates.py (Python)

Code before:
def assert_set_equal(result, expected, path=(), msg='', **kwargs):
    _check_sets(
        result,
        expected,
        msg,
        path,
        'element',
    )


@assert_equal.register(np.ndarray, np.ndarray)
def assert_array_equal(result,
                       expected,
                       path=(),
                       msg='',
                       array_verbose=True,
                       array_decimal=None,
                       **kwargs):
    f = (
        np.testing.assert_array_equal
        if array_decimal is None else
        partial(np.testing.assert_array_almost_equal, decimal=array_decimal)
    )

    if result.dtype.kind == 'M' and expected.dtype.kind == 'M':
        is_datetime = True
        result = result.astype('int64')
        expected = expected.astype('int64')
    else:
        is_datetime = False

    try:
        f(
            result,
            expected,
            verbose=array_verbose,
            err_msg=msg,
        )
    except AssertionError as e:
        if is_datetime:
            raise AssertionError(
                "\nArrays are not equal\n\n"
                "result: %s\n"
                "expected: %s\n"
                "%s" % (
                    np.array_repr(result.astype('M8[ns]')),
                    np.array_repr(expected.astype('M8[ns]')),
                    _fmt_path(path),
                )

            )

Review comment (suggestion):
if only one of the arrays has 'M' dtype, we can let `assert_array_equal` handle the mismatch.

Code after:
@assert_equal.register(set, set)
def assert_set_equal(result, expected, path=(), msg='', **kwargs):
    _check_sets(
        result,
        expected,
        msg,
        path,
        'element',
    )


@assert_equal.register(np.ndarray, np.ndarray)
def assert_array_equal(result,
                       expected,
                       path=(),
                       msg='',
                       array_verbose=True,
                       array_decimal=None,
                       **kwargs):
    result_dtype = result.dtype
    expected_dtype = expected.dtype

    if result_dtype.kind in 'mM' and expected_dtype.kind in 'mM':
        assert result_dtype == expected_dtype, (
            "\nType mismatch:\n\n"
            "result dtype: %s\n"
            "expected dtype: %s\n%s"
            % (result_dtype, expected_dtype, _fmt_path(path))
        )
        f = partial(
            np.testing.utils.assert_array_compare,
            compare_datetime_arrays,
            header='Arrays are not equal',
        )
    elif array_decimal is not None:
        f = partial(
            np.testing.assert_array_almost_equal,
            decimal=array_decimal,
        )
    else:
        f = np.testing.assert_array_equal

    try:
        f(
            result,
            expected,
            verbose=array_verbose,
            err_msg=msg,
        )
    except AssertionError as e:
        raise AssertionError('\n'.join((str(e), _fmt_path(path))))
Repository: quantopian/zipline
File: zipline/testing/predicates.py (Python)

Code before:
        expected,
        msg,
        path,
        'element',
    )


@assert_equal.register(np.ndarray, np.ndarray)
def assert_array_equal(result,
                       expected,
                       path=(),
                       msg='',
                       array_verbose=True,
                       array_decimal=None,
                       **kwargs):
    f = (
        np.testing.assert_array_equal
        if array_decimal is None else
        partial(np.testing.assert_array_almost_equal, decimal=array_decimal)
    )

    is_datetime = False
    if result.dtype.kind == 'M' and expected.dtype.kind == 'M':
        is_datetime = True
        result = result.astype('int64')
        expected = expected.astype('int64')

    try:
        f(
            result,
            expected,
            verbose=array_verbose,
            err_msg=msg,
        )
    except AssertionError as e:
        if is_datetime:
            raise AssertionError(
                "\nArrays are not equal\n\n"
                "result: %s\n"
                "expected: %s\n"
                "%s" % (
                    np.array_repr(result.astype('M8[ns]')),
                    np.array_repr(expected.astype('M8[ns]')),
                    _fmt_path(path),
                )

            )
        raise AssertionError('\n'.join((str(e), _fmt_path(path))))


@assert_equal.register(LabelArray, LabelArray)

Review comment (suggestion):
Hmm. One thing we might want to handle here is if result and expected are both datetime dtype, but have different units. I'm not sure if we'd want those to compare the same.

Code after:
        result,
        expected,
        msg,
        path,
        'element',
    )


@assert_equal.register(np.ndarray, np.ndarray)
def assert_array_equal(result,
                       expected,
                       path=(),
                       msg='',
                       array_verbose=True,
                       array_decimal=None,
                       **kwargs):
    result_dtype = result.dtype
    expected_dtype = expected.dtype

    if result_dtype.kind in 'mM' and expected_dtype.kind in 'mM':
        assert result_dtype == expected_dtype, (
            "\nType mismatch:\n\n"
            "result dtype: %s\n"
            "expected dtype: %s\n%s"
            % (result_dtype, expected_dtype, _fmt_path(path))
        )
        f = partial(
            np.testing.utils.assert_array_compare,
            compare_datetime_arrays,
            header='Arrays are not equal',
        )
    elif array_decimal is not None:
        f = partial(
            np.testing.assert_array_almost_equal,
            decimal=array_decimal,
        )
    else:
        f = np.testing.assert_array_equal

    try:
        f(
            result,
            expected,
            verbose=array_verbose,
            err_msg=msg,
        )
    except AssertionError as e:
        raise AssertionError('\n'.join((str(e), _fmt_path(path))))


@assert_equal.register(LabelArray, LabelArray)
Repository: quantopian/zipline
File: zipline/pipeline/factors/basic.py (Python)

Code before:


class PeerCount(SingleInputMixin, CustomFactor):
    """
    Peer Count of distinct categories in a given classifier.  This factor
    is returned by the classifier instance method peer_count()

    **Default Inputs:** None

    **Default Window Length:** 1
    """
    window_length = 1
    missing_value = -1
    dtype = int64_dtype

    def _validate(self):
        super(PeerCount, self)._validate()
        if self.window_length != 1:
            raise ValueError(
                "'PeerCount' expected a window length of 1, but was given"
                "{window_length}.".format(window_length=self.window_length)
            )

    def compute(self, today, assets, out, arrays):
        # Convert arrays to group label int arrays
        group_labels, _ = self.inputs[0]._to_integral(arrays[0])
        _, inverse, counts = unique(  # Get counts, idx of unique groups
            group_labels,
            return_counts=True,
            return_inverse=True,
        )
        out[:] = counts[inverse]


# Convenience aliases
EWMA = ExponentialWeightedMovingAverage
EWMSTD = ExponentialWeightedMovingStdDev


Review comment (suggestion):
Not your doing, but I'm surprised to see `_to_integral` as a private method. I suppose we don't expect users to ever call it on classifiers in an actual algorithm.

Code after:


class PeerCount(SingleInputMixin, CustomFactor):
    """
    Peer Count of distinct categories in a given classifier.  This factor
    is returned by the classifier instance method peer_count()

    **Default Inputs:** None

    **Default Window Length:** 1
    """
    window_length = 1
    missing_value = -1
    dtype = int64_dtype

    def _validate(self):
        super(PeerCount, self)._validate()
        if self.window_length != 1:
            raise ValueError(
                "'PeerCount' expected a window length of 1, but was given"
                "{window_length}.".format(window_length=self.window_length)
            )

    def compute(self, today, assets, out, classifier_values):
        # Convert classifier array to group label int array
        group_labels, _ = self.inputs[0]._to_integral(classifier_values[0])
        _, inverse, counts = unique(  # Get counts, idx of unique groups
            group_labels,
            return_counts=True,
            return_inverse=True,
        )
        out[:] = counts[inverse]


# Convenience aliases
EWMA = ExponentialWeightedMovingAverage
EWMSTD = ExponentialWeightedMovingStdDev
Repository: quantopian/zipline
File: zipline/pipeline/factors/basic.py (Python)

Code before:
    **Default Inputs:** None

    **Default Window Length:** 1
    """
    window_length = 1
    missing_value = nan

    def _validate(self):
        super(PeerCount, self)._validate()
        if self.window_length != 1:
            raise ValueError(
                "'PeerCount' expected a window length of 1, but was given"
                "{window_length}.".format(window_length=self.window_length)
            )

    def compute(self, today, assets, out, classifier_values):
        # Convert classifier array to group label int array
        group_labels, null_label = self.inputs[0]._to_integral(
            classifier_values[0]
        )
        _, inverse, counts = unique(  # Get counts, idx of unique groups
            group_labels,
            return_counts=True,
            return_inverse=True,
        )
        out[:] = where(
            group_labels != null_label,
            counts[inverse],
            self.missing_value,
        )


# Convenience aliases
EWMA = ExponentialWeightedMovingAverage
EWMSTD = ExponentialWeightedMovingStdDev


Review comment (performance):
I think slightly more efficient here would be:

```python
np.copyto(out, counts[inverse], group_labels != null_label)
```

```
In [9]: %timeit a[mask] = cp[mask]
The slowest run took 5.88 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 44.6 µs per loop

In [10]: %timeit np.copyto(a, cp, where=mask)
The slowest run took 5.96 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 24.2 µs per loop
```

Code after:

    **Default Window Length:** 1
    """
    window_length = 1

    def _validate(self):
        super(PeerCount, self)._validate()
        if self.window_length != 1:
            raise ValueError(
                "'PeerCount' expected a window length of 1, but was given"
                "{window_length}.".format(window_length=self.window_length)
            )

    def compute(self, today, assets, out, classifier_values):
        # Convert classifier array to group label int array
        group_labels, null_label = self.inputs[0]._to_integral(
            classifier_values[0]
        )
        _, inverse, counts = unique(  # Get counts, idx of unique groups
            group_labels,
            return_counts=True,
            return_inverse=True,
        )
        copyto(out, counts[inverse], where=(group_labels != null_label))


# Convenience aliases
EWMA = ExponentialWeightedMovingAverage
EWMSTD = ExponentialWeightedMovingStdDev
Repository: quantopian/zipline
File: tests/pipeline/test_column.py (Python)

Code before:
    ])
    def test_comparison_errors(self, op):
        for column in TDS.columns:
            with self.assertRaises(TypeError):
                op(column, 1000)
            with self.assertRaises(TypeError):
                op(1000, column)
            with self.assertRaises(TypeError):
                op(column, 'test')
            with self.assertRaises(TypeError):
                op('test', column)

    def test_comparison_error_message(self):
        column = USEquityPricing.volume
        err_msg = (
            "'<' not supported between instance of"
            " 'int' and 'EquityPricing<US>.volume'."
            " Did you mean to use 'EquityPricing<US>.volume.latest'?"
        )

        with self.assertRaises(TypeError) as e:
            column < 1000
        self.assertEqual(str(e.exception), err_msg)

        with self.assertRaises(TypeError) as e:
            1000 > column
        self.assertEqual(str(e.exception), err_msg)

        try:
            column.latest < 1000
        except Exception:
            self.fail()


Review comment (refactor):
Hmm... I was hoping we'd always be able to display the correct operator symbol in the error message; but I guess we can't actually know which one it is without inspecting the stack, since Python just uses `__lt__` instead of `__rgt__`. (It seems reasonable that the interpreter would make use of its own stack to get that information when generating the built-in errors in Python 3.7+, though.)

Since the `<` symbol in this error message is not the same as the `>` symbol in the code that caused it, I think it's probably better to not display a symbol at all, rather than one that's only correct in half the cases. On the plus side, it should simplify the code a bit. I'll go ahead and make the change; let me know if you have other ideas, though.

Code after:
    ])
    def test_comparison_errors(self, op):
        for column in TDS.columns:
            with self.assertRaises(TypeError):
                op(column, 1000)
            with self.assertRaises(TypeError):
                op(1000, column)
            with self.assertRaises(TypeError):
                op(column, 'test')
            with self.assertRaises(TypeError):
                op('test', column)

    def test_comparison_error_message(self):
        column = USEquityPricing.volume
        err_msg = (
            "Can't compare 'EquityPricing<US>.volume' with 'int'."
            " (Did you mean to use '.latest'?)"
        )

        with self.assertRaises(TypeError) as e:
            column < 1000
        self.assertEqual(str(e.exception), err_msg)

        try:
            column.latest < 1000
        except TypeError:
            self.fail()
Repository: quantopian/zipline
File: tests/pipeline/test_dataset.py (Python)

Code before:
from textwrap import dedent

from zipline.pipeline.data.dataset import Column, DataSet
from zipline.testing import chrange, ZiplineTestCase
from zipline.testing.predicates import assert_messages_equal
from zipline.utils.numpy_utils import datetime64ns_dtype


class SomeDataSet(DataSet):
    a = Column(dtype=float)
    b = Column(dtype=object)
    c = Column(dtype=int, missing_value=-1)

    exists_but_not_a_column = 'foo'


# A DataSet with lots of columns.
class LargeDataSet(DataSet):
    locals().update({
        name: Column(dtype=float)
        for name in chrange('a', 'z')
    })


class InvalidColumnDataset(DataSet):
    currency_aware_date = Column(dtype=datetime64ns_dtype, currency_aware=True)


class GetColumnTestCase(ZiplineTestCase):

    def test_get_column_success(self):
        a = SomeDataSet.a
        b = SomeDataSet.b
        c = SomeDataSet.c

        # Run multiple times to validate caching of descriptor return values.
        for _ in range(3):
            self.assertIs(SomeDataSet.get_column('a'), a)
            self.assertIs(SomeDataSet.get_column('b'), b)
            self.assertIs(SomeDataSet.get_column('c'), c)

    def test_get_column_failure(self):
        with self.assertRaises(AttributeError) as e:
            SomeDataSet.get_column('arglebargle')

        result = str(e.exception)
        expected = dedent(
            """\
            SomeDataSet has no column 'arglebargle':

            Possible choices are:

Review comment (suggestion):
Talked about this in person, but I think we want this statement to error rather than the attribute access.

Code after:
from textwrap import dedent

from zipline.pipeline.data.dataset import Column, DataSet
from zipline.testing import chrange, ZiplineTestCase
from zipline.testing.predicates import assert_messages_equal


class SomeDataSet(DataSet):
    a = Column(dtype=float)
    b = Column(dtype=object)
    c = Column(dtype=int, missing_value=-1)

    exists_but_not_a_column = 'foo'


# A DataSet with lots of columns.
class LargeDataSet(DataSet):
    locals().update({
        name: Column(dtype=float)
        for name in chrange('a', 'z')
    })


class GetColumnTestCase(ZiplineTestCase):

    def test_get_column_success(self):
        a = SomeDataSet.a
        b = SomeDataSet.b
        c = SomeDataSet.c

        # Run multiple times to validate caching of descriptor return values.
        for _ in range(3):
            self.assertIs(SomeDataSet.get_column('a'), a)
            self.assertIs(SomeDataSet.get_column('b'), b)
            self.assertIs(SomeDataSet.get_column('c'), c)

    def test_get_column_failure(self):
        with self.assertRaises(AttributeError) as e:
            SomeDataSet.get_column('arglebargle')

        result = str(e.exception)
        expected = dedent(
            """\
            SomeDataSet has no column 'arglebargle':

            Possible choices are:
              - a
              - b
              - c"""
        )
        assert_messages_equal(result, expected)
Repository: quantopian/zipline
File: zipline/__main__.py (Python)

Code before:
    '-D',
    '--define',
    multiple=True,
    help="Define a name to be bound in the namespace before executing"
         " the algotext. For example '-Dname=value'. The value may be any "
         "python expression. These are evaluated in order so they may refer "
         "to previously defined names.",
)
@click.option(
    '--data-frequency',
    type=click.Choice({'daily', 'minute'}),
    default='daily',
    show_default=True,
    help='The data frequency of the simulation.',
)
@click.option(
    '--capital-base',
    type=float,
    default=10e6,
    show_default=True,
    help='The starting capital for the simulation.',
)
@click.option(
    '-b',
    '--bundle',
    default='quantiopian-quandl',
    metavar='BUNDLE-NAME',
    show_default=True,
    help='The data bundle to use for the simulation.',
)
@click.option(
    '--bundle-timestamp',
    type=Timestamp(),
    default=pd.Timestamp.utcnow(),
    show_default=False,
    help='The date to lookup data on or before.\n'
         '[default: <current-time>]'
)
@click.option(
    '-bf',
    '--benchmark-file',
    default=None,
    type=click.Path(exists=True, dir_okay=False, readable=True, path_type=str),
    help='The csv file that contains the benchmark returns',
)
@click.option(
    '--benchmark-symbol',
    default=None,
    type=click.STRING,
    help="The symbol of the instrument to be used as a benchmark "
         "(should exist in the ingested bundle)",

Review comment (suggestion):
```suggestion
    default='quantopian-quandl',
```

Code after:
    '-D',
    '--define',
    multiple=True,
    help="Define a name to be bound in the namespace before executing"
         " the algotext. For example '-Dname=value'. The value may be any "
         "python expression. These are evaluated in order so they may refer "
         "to previously defined names.",
)
@click.option(
    '--data-frequency',
    type=click.Choice({'daily', 'minute'}),
    default='daily',
    show_default=True,
    help='The data frequency of the simulation.',
)
@click.option(
    '--capital-base',
    type=float,
    default=10e6,
    show_default=True,
    help='The starting capital for the simulation.',
)
@click.option(
    '-b',
    '--bundle',
    default='quantopian-quandl',
    metavar='BUNDLE-NAME',
    show_default=True,
    help='The data bundle to use for the simulation.',
)
@click.option(
    '--bundle-timestamp',
    type=Timestamp(),
    default=pd.Timestamp.utcnow(),
    show_default=False,
    help='The date to lookup data on or before.\n'
         '[default: <current-time>]'
)
@click.option(
    '-bf',
    '--benchmark-file',
    default=None,
    type=click.Path(exists=True, dir_okay=False, readable=True, path_type=str),
    help='The csv file that contains the benchmark returns',
)
@click.option(
    '--benchmark-symbol',
    default=None,
    type=click.STRING,
    help="The symbol of the instrument to be used as a benchmark "
         "(should exist in the ingested bundle)",
Repository: rails/rails
File: activestorage/app/controllers/concerns/active_storage/set_headers.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveStorage::SetHeaders #:nodoc:
  extend ActiveSupport::Concern

  private
    def set_headers(blob)
      expires_in 1.year, public: true

      response.headers["Content-Type"] = blob.content_type
      response.headers["Content-Disposition"] = ActionDispatch::Http::ContentDisposition.format(
        disposition: params[:disposition] || "inline",
        filename: blob.filename.sanitized
      )
    end
end


Review comment (security):
`set_headers` is vague for what this method does. I think I’d pull `expires_in` out and rename this to `set_content_headers_from_blob` or similar:

```ruby
# Here:
def set_content_headers_from(blob)
  response.headers["Content-Type"] = blob.content_type
  response.headers["Content-Disposition"] = ActionDispatch::Http::ContentDisposition.format \
    disposition: params[:disposition] || "inline", filename: blob.filename.sanitized
end
```

```ruby
# activestorage/app/controllers/active_storage/blobs/proxy_controller.rb
def show
  expires_in 1.hour, public: true
  set_content_headers_from @blob
  stream @blob
end

private
  # Good candidate for hoisting to the base class and reusing elsewhere.
  def stream(blob)
    blob.download do |chunk|
      response.stream.write chunk
    end
  ensure
    response.stream.close
  end
```
   

Code after:
# frozen_string_literal: true

module ActiveStorage::SetHeaders #:nodoc:
  extend ActiveSupport::Concern

  private
    def set_content_headers_from(blob)
      response.headers["Content-Type"] = blob.content_type
      response.headers["Content-Disposition"] = ActionDispatch::Http::ContentDisposition.format \
        disposition: params[:disposition] || "inline", filename: blob.filename.sanitized
    end
end