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

Should we just go ahead and use SVector in position and velocity fields of ContinuousSpace...? #672

Closed
Datseris opened this issue Aug 5, 2022 · 4 comments · Fixed by #846
Labels
continuous Continuous space related discussion quality of life QoL enhancements that make user experience smoother

Comments

@Datseris
Copy link
Member

Datseris commented Aug 5, 2022

Alright, so, plenty of times in many use cases of ContinuousSpace, it becomes an obvious pain to use NTuple instead of SVector. So, should we just go ahead and change the types to static vectors? Problem is, I am not sure how breaking this is. Unless a user for whatever reason annotatted the position type, everything the user did before will work now as well. This includes changing velocity via a tuple, agent.vel = (1, 2) would still work even if vel isa SVector.

For example, this problem came when making the Flocking example, in the discussion in #671 , in the integration example with CellListMap.jl, the bacteria example, I mean, practically everywhere with ContinuousSpace.

@Datseris Datseris added discussion continuous Continuous space related quality of life QoL enhancements that make user experience smoother labels Aug 5, 2022
@Datseris
Copy link
Member Author

Datseris commented Aug 5, 2022

@mastrof perhaps you want to start a PR that does this change, so we can get an estimate of what things may be breaking?

@AayushSabharwal
Copy link
Collaborator

AayushSabharwal commented Aug 6, 2022

Unfortunately, I think this is breaking, simply because before the @agent change our API allows writing structs by hand, which means people probably did annotate the type. I know I've done it, simply because I don't like the squiggly underline VSCode puts whenever I use @agent. Bar that, I think most if not all code should just work with SVector{2} as well, since .+ and .* do the same thing in both cases. The only exception off the top of my head is using add_agent! and passing NTuple for the agent position.

We could just use a union type internally and typecast it to SVector every time we modify the struct

@mastrof
Copy link
Contributor

mastrof commented Aug 6, 2022

Sure, happy to start a PR, should be able to do it tomorrow (actually today, since midnight just clocked here).
So, if I get what you are after, I'll change the ContinuousAgent field types from NTuple to SVector, and consequently all the function signatures that expect a NTuple from a ContinuousAgent will ask for a SVector instead.

For the limited knowledge I have of the library, spaces/continuous.jl and spaces/utilities.jl seem to be the only "obvious" affected files (+ Agents.jl just to import StaticArrays).

@Datseris
Copy link
Member Author

Datseris commented Aug 7, 2022

and consequently all the function signatures that expect a NTuple from a ContinuousAgent will ask for a SVector instead.

Furthermore you need to add SVector to ValidPos. I don't remember where the definition of ValidPos is, I think in model.jl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous Continuous space related discussion quality of life QoL enhancements that make user experience smoother
Projects
None yet
3 participants