Skip to content
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.

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

Surprising behavior of append!(..) #113

Closed
MartinOtter opened this issue Aug 12, 2021 · 7 comments
Closed

Surprising behavior of append!(..) #113

MartinOtter opened this issue Aug 12, 2021 · 7 comments

Comments

@MartinOtter
Copy link

append!(..) seems to be not working correctly with MonteCarloMeasurements (I am using Julia 1.6.2, MonteCarloMeasurements 0.10.4). The code

module TestAppend

using MonteCarloMeasurements

s1 = 1.0
s2 = [1.0, 2.0]
s3 = Particles(3)
s4 = [s3, 2*s3]

v1 = typeof(s1)[]
v2 = typeof(s3)[]

@show s1
@show s2
@show s3
@show s4
@show v1
@show v2

append!(v1,s1)
append!(v1,s2)

append!(v2,s3)
append!(v2,s4)

@show v1
@show v2

produces the following output

julia> include("TestAppend.jl")
s1 = 1.0
s2 = [1.0, 2.0]
s3 = 0.0 ± 0.97
s4 = MonteCarloMeasurements.Particles{Float64, 3}[0.0 ± 0.97, 0.0 ± 1.9]
v1 = Float64[]
v2 = MonteCarloMeasurements.Particles{Float64, 3}[]
v1 = [1.0, 1.0, 2.0]
v2 = MonteCarloMeasurements.Particles{Float64, 3}[0.967, 0.0, -0.967, 0.0 ± 0.97, 0.0 ± 1.9]

I would have expected that v3 has three Particles and not three Float64 values and two particles.

@MartinOtter
Copy link
Author

I found a work around:

Base.append!(v::Vector{T}, s::T) where {T<:MonteCarloMeasurements.Particles} = Base.push!(v,s)

but I hope this can be fixed in a better way.

@baggepinnen
Copy link
Owner

Hello Martin,

I assume you mean that your expect v2 (not v3) to have only three particle elements?

First note that v2 contains only particles, the printing is just shorter for particles that have no variability. Also note that

append!(v1,s1)
append!(v2,s3)

is incorrect usage of append! according to it's docstring, since you are appending scalars and not collections.

Use push! to add individual items to collection which are not already themselves in another collection.

The usage above leads to the incorrect result due to a fallback in Base that iterates the argument to be appended and since particles are iterable, you get unexpected results. It can be debated whether or not particles should be iterable, perhaps they should not, in which case the problem would go away.

@MartinOtter
Copy link
Author

Yes, I meant v2.

In my application, code is generated and when generating code it is not known whether the variable to be appended is a scalar or a vector.

Its not critical, since I have a workaround.

It would be nice, if there would be no exception in such a corner case.

Maybe its related: When developing code and then switch the type (using MonteCarloMeasurements), its annoying that length(s3) returns 3 and not 1 (although length(s1) returns 1). I had therefore to introduce at various places checks whether the type is MonteCarloMeasurements or not and then implement different code.

@baggepinnen
Copy link
Owner

I agree, these differences between a regular Float64 scalar and Particles are very annoying, and it's on the roadmap to version 1.0 to change the behavior so that Particles always behave like scalars. You'll find more on this discussion here #88 (comment)

@baggepinnen
Copy link
Owner

@MartinOtter On master I know have

julia> append!(v2,s4)
3-element Vector{Particles{Float64, 3}}:
 0.0 ± 0.967
 0.0 ± 0.967
 0.0 ± 1.93

which I hope is what you expected?
Similarly,

julia> s3
0.0 ± 0.967

julia> length(s3)
1

So to conclude, Particles should now behave much more like scalars. A new release will be out within a few days, I just want to make sure I've made all the breaking changes I want to make before making a 1.0 release.

@MartinOtter
Copy link
Author

Thanks very much. Yes, this is the expected behavior

@baggepinnen
Copy link
Owner

Version 1.0 of MCM is now released
https://github.com/baggepinnen/MonteCarloMeasurements.jl/releases/tag/v1.0.0
I hope this release makes integrating Particles much smoother!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants