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

Update abstract.jl solves #1045 #1059

Merged
merged 3 commits into from
Jul 17, 2024
Merged

Update abstract.jl solves #1045 #1059

merged 3 commits into from
Jul 17, 2024

Conversation

amynang
Copy link
Contributor

@amynang amynang commented Jul 15, 2024

The culprit behind #1045 was the use of poly! for user defined polygons. As a first step, I replaced it with scatter!(p, p.pos; p.color, p.marker, p.markersize, p.agentsplotkwargs...) and generating the plot with the custom polygon worked. In that case I think the whole if else statement is no longer necessary so I removed it.

if user_used_polygons(p.agent_marker[], p.marker[])
poly!(p, p.marker; p.color, p.marker, p.markersize, p.agentsplotkwargs...)
else
scatter!(p, p.pos; p.color, p.marker, p.markersize, p.agentsplotkwargs...)
end

The culprit behind JuliaDynamics#1045 was the use of poly! for user defined polygons. As a first step, I replaced it with 
`scatter!(p, p.pos; p.color, p.marker, p.markersize, p.agentsplotkwargs...)` and generating the plot with the custom polygon worked. In that case I think the whole if else statement is no longer necessary so I removed it.

```
if user_used_polygons(p.agent_marker[], p.marker[])
poly!(p, p.marker; p.color, p.marker, p.markersize, p.agentsplotkwargs...)
else
scatter!(p, p.pos; p.color, p.marker, p.markersize, p.agentsplotkwargs...)
end
```
@amynang amynang changed the title Update abstract.jl solves #1015 Update abstract.jl solves #1045 Jul 15, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.53%. Comparing base (8b5b456) to head (d902e39).
Report is 125 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1059       +/-   ##
===========================================
+ Coverage   70.12%   86.53%   +16.41%     
===========================================
  Files          42       39        -3     
  Lines        2718     2578      -140     
===========================================
+ Hits         1906     2231      +325     
+ Misses        812      347      -465     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amynang
Copy link
Contributor Author

amynang commented Jul 15, 2024

I just realized that while this change removes the error that abmvideo was throwing, the end result is not quite right; the generated video seems to show only part of the space. This is despite the fact that the x and y axes suggest that the dimensions are correct. I make a model with 100 agents but see only few of them and it is also clear that they don't reappear where you'd expect them to given a periodic space. I do not understand what is going on here.

@Tortar
Copy link
Member

Tortar commented Jul 15, 2024

I didn't test anything but I feel like removing poly! shouldn't be the solution, what does the error say when you try to create the video with the previous code?

@Tortar
Copy link
Member

Tortar commented Jul 15, 2024

ah also, thanks for your help on this :-)

@Datseris
Copy link
Member

unfortunately i have to agree, removing poly! also doesn't sound like a solution to me. Does the scatter! actually plot the polygons..? Does the flocking video looks like flocking, with triangles moving and rotating around? If not, then we haven't solved the problem.

I suspect that this code needs to stay as is, with if:

if user_used_polygons(p.agent_marker[], p.marker[])
poly!(p, p.marker; p.color, p.marker, p.markersize, p.agentsplotkwargs...)
else
scatter!(p, p.pos; p.color, p.marker, p.markersize, p.agentsplotkwargs...)
end

but what needs to be done is that we cannot blindly propagate p.agentsplotkwargs anymore. There are some keywords that poly! doesn't accept that are otherwise valid for scatter!, so we have to filter them out.

@amynang
Copy link
Contributor Author

amynang commented Jul 15, 2024

@Tortar here is the full error message with the original code. This is the model where I encountered the problem.

@Datseris scatter! does actually plot the polygons. The first picture is a screenshot with the PR code, the next one is with the agent_marker argument in abmvideo commented out. This is the strange behavior I described earlier, both cases have 100 agents but the first looks zoomed into a part of the space for some reason.
pr1
pr0

@Datseris
Copy link
Member

agent_marker argument in abmvideo commented out

Don't do that, it doesn't make sense at the moment. So we don't have to spend resources figuring out what commenting this out does.

I didn't know that scatter! plots polygons. Then we can just use scatter! for everything. However, let's make sure about one more thing: if you increase the polygon size, for example by multiplying the polygon coordinates with some factor

const bird_polygon = Makie.Polygon(5 .* Point2f[(-1, -1), (2, 0), (-1, 1)])
function bird_marker(b::Bird)
    φ = atan(b.vel[2], b.vel[1]) #+ π/2 + π
    rotate_polygon(bird_polygon, φ)
end

(notice 5 .*)

Are the polygons larger in size in scatter!? Does it respect the actual size of the polygons?

@amynang
Copy link
Contributor Author

amynang commented Jul 15, 2024

Yep, polygons get larger
pr2

@Tortar
Copy link
Member

Tortar commented Jul 15, 2024

I tried your model, and with the last commit I pushed everything works fine, we don't propagate to poly! anymore markersize, but maybe it never worked? From the code of @Datseris it seems so because you define the markersize when creating the polygon

@amynang
Copy link
Contributor Author

amynang commented Jul 16, 2024

Great! Thanks a lot!

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

Alright, please go into top-level Project.toml and increment the patch version (3rd number) by one. Then we can merge and immediatelly tag a new release.

@Tortar Tortar merged commit 62601fb into JuliaDynamics:main Jul 17, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

4 participants