Skip to content

fix: add accumulator method for (size,size) #3248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jokasimr merged 2 commits into from
Sep 25, 2023
Merged

fix: add accumulator method for (size,size) #3248

jokasimr merged 2 commits into from
Sep 25, 2023

Conversation

@jokasimr
Copy link
Contributor

@jokasimr jokasimr commented Sep 14, 2023 •
edited
Loading

Fixes #3125.

Should I add a test for size_of for a Variable containing many strings?
What is the c++ api equivalent of sc.scalar('abc').broadcast(dims=('x',), shape=(1000000,))?

@jokasimr jokasimr requested a review from jl-wynen September 14, 2023 11:51
SimonHeybrock
SimonHeybrock reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@SimonHeybrock SimonHeybrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add a test for size_of for a Variable containing many strings?
What is the c++ api equivalent of sc.scalar('abc').broadcast(dims=('x',), shape=(1000000,))?

You need something like

Dimensions dims(Dim::X, 1000000);
broadcast(makeVariable<std::string>(Values{'abc'}), dims);

iirc

lib/dataset/util.cpp
out += sizeof(std::string) + str.size();
}
}
void operator()(scipp::index &out, const scipp::index &s) { out += s; }
Copy link
Member

@SimonHeybrock SimonHeybrock Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you could have put this in the overloaded above, by adding another lambda there. Then the diff would be much smaller.

@jokasimr
Copy link
Contributor Author

jokasimr commented Sep 14, 2023 

D:\a\scipp\scipp\lib\dataset\test\size_of_test.cpp(155): error: Expected equality of these values:
  size_of(var, SizeofTag::ViewOnly)
    Which is: 35000256
  view_size
    Which is: 32000256
D:\a\scipp\scipp\lib\dataset\test\size_of_test.cpp(156): error: Expected equality of these values:
  size_of(var, SizeofTag::Underlying)
    Which is: 291
  str_size + object_size
    Which is: 288
[  FAILED  ] SizeOf.variable_of_many_strings (1 ms)

Interesting @jl-wynen, here we get the opposite (this is Windows).

@jl-wynen
Copy link
Member

jl-wynen commented Sep 14, 2023

The small string optimisation depends on the standard library implementation. It is possible that the Windows library doesn't do it. To circumvent, you could use a long string to ensure a heap allocation. Something like 10-20 elements should do.

@jokasimr jokasimr force-pushed the fix-sizeof-large-str branch from e3fd9da to a2d5553 Compare September 14, 2023 14:25
@jokasimr jokasimr force-pushed the fix-sizeof-large-str branch from 4f6050f to b1741ce Compare September 25, 2023 07:41
@jokasimr jokasimr merged commit 0054cb7 into main Sep 25, 2023
@jokasimr jokasimr deleted the fix-sizeof-large-str branch September 25, 2023 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SimonHeybrock SimonHeybrock SimonHeybrock left review comments

@jl-wynen jl-wynen jl-wynen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Exception from HTML repr for variable with many strings

4 participants

@jokasimr @jl-wynen @SimonHeybrock