issue_comments

Table actions

6 rows where issue = 1423321834 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

Suggested facets: reactions, created_at (date), updated_at (date)

user 3

author_association 2

issue 1

  • Actually make the fast code path return early for Aligner.align · 6 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
1295193308 https://github.com/pydata/xarray/pull/7222#issuecomment-1295193308 https://api.github.com/repos/pydata/xarray/issues/7222 IC_kwDOAMm_X85NMxTc dcherian 2448579 2022-10-28T16:22:24Z 2022-10-28T16:22:24Z MEMBER

before after ratio [040816a6] [cd40c8a0] - 486±6μs 311±5μs 0.64 merge.DatasetAddVariable.time_variable_insertion(10) - 19.1±2ms 12.0±0.2ms 0.63 merge.DatasetAddVariable.time_variable_insertion(1000) - 2.18±0.02ms 1.34±0.03ms 0.62 merge.DatasetAddVariable.time_variable_insertion(100)

nice work @hmaarrfk !

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Actually make the fast code path return early for Aligner.align 1423321834
1293689561 https://github.com/pydata/xarray/pull/7222#issuecomment-1293689561 https://api.github.com/repos/pydata/xarray/issues/7222 IC_kwDOAMm_X85NHCLZ hmaarrfk 90008 2022-10-27T15:15:45Z 2022-10-27T15:15:45Z CONTRIBUTOR

but that would be a lot of work especially for such a critical piece of code in Xarray.

Agreed. I'll take the small wins where I can :D.

Great! I think this will be a good addition with: https://github.com/pydata/xarray/pull/7223#discussion_r1007023769

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Actually make the fast code path return early for Aligner.align 1423321834
1293624950 https://github.com/pydata/xarray/pull/7222#issuecomment-1293624950 https://api.github.com/repos/pydata/xarray/issues/7222 IC_kwDOAMm_X85NGyZ2 benbovy 4160723 2022-10-27T14:37:10Z 2022-10-27T14:37:10Z MEMBER

Thanks @hmaarrfk!

I think the rapid return, helps by about 40% is still pretty good.

Yes definitely. I think we just forgot to add it.

However, I will argue that Aligner should really not be a class.

The reason of using a class is mainly for better code readability and also so that it is easier to refactor later. The alignment logic is really complex with lots of intermediate objects that are created and/or used at various stages. Probably using functions with some custom containers would have achieved the same goal, to be fair. This part of Xarray internals still deserves to be improved, but that would be a lot of work especially for such a critical piece of code in Xarray.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Actually make the fast code path return early for Aligner.align 1423321834
1291405225 https://github.com/pydata/xarray/pull/7222#issuecomment-1291405225 https://api.github.com/repos/pydata/xarray/issues/7222 IC_kwDOAMm_X85M-Uep hmaarrfk 90008 2022-10-26T02:19:23Z 2022-10-26T02:19:23Z CONTRIBUTOR

I think the rapid return, helps by about 40% is still pretty good.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Actually make the fast code path return early for Aligner.align 1423321834
1291402576 https://github.com/pydata/xarray/pull/7222#issuecomment-1291402576 https://api.github.com/repos/pydata/xarray/issues/7222 IC_kwDOAMm_X85M-T1Q hmaarrfk 90008 2022-10-26T02:17:45Z 2022-10-26T02:17:45Z CONTRIBUTOR

hmm ok. it seems i can't blatently avoid the copy like that.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Actually make the fast code path return early for Aligner.align 1423321834
1291391647 https://github.com/pydata/xarray/pull/7222#issuecomment-1291391647 https://api.github.com/repos/pydata/xarray/issues/7222 IC_kwDOAMm_X85M-RKf hmaarrfk 90008 2022-10-26T02:03:41Z 2022-10-26T02:03:41Z CONTRIBUTOR

The reason this is a separate merge request, is that I agree that this is more contentious as a change.

However, I will argue that Aligner should really not be a class.

Using ripgrep you find that the only instances of Aligner exist internally: ` xarray/core/dataset.py 2775: aligner: alignment.Aligner, 2783: """Callback called fromAligner`` to create a new reindexed Dataset."""

xarray/core/alignment.py 107:class Aligner(Generic[DataAlignable]): 114: aligner = Aligner(objects, *kwargs) <------- Example 767: aligner = Aligner( <----------- Used and consumed for the method align 881: aligner = Aligner( <----------- Used and consumed for the method reindex 909: # This check is not performed in Aligner.

xarray/core/dataarray.py 1752: aligner: alignment.Aligner, 1760: """Callback called from Aligner to create a new reindexed DataArray.""" ```

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Actually make the fast code path return early for Aligner.align 1423321834

Advanced export

JSON shape: default, array, newline-delimited, object

CSV options: 

CREATE TABLE [issue_comments] (
   [html_url] TEXT,
   [issue_url] TEXT,
   [id] INTEGER PRIMARY KEY,
   [node_id] TEXT,
   [user] INTEGER REFERENCES [users]([id]),
   [created_at] TEXT,
   [updated_at] TEXT,
   [author_association] TEXT,
   [body] TEXT,
   [reactions] TEXT,
   [performed_via_github_app] TEXT,
   [issue] INTEGER REFERENCES [issues]([id])
);
CREATE INDEX [idx_issue_comments_issue]
    ON [issue_comments] ([issue]);
CREATE INDEX [idx_issue_comments_user]
    ON [issue_comments] ([user]);