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

overloading + to combine rules and rulesets #176

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gottacatchenall
Copy link

No description provided.

@gottacatchenall gottacatchenall marked this pull request as ready for review July 8, 2021 23:10
@rafaqz
Copy link
Member

rafaqz commented Jul 9, 2021

This looks good to me. Maybe apply https://github.com/invenia/BlueStyle which means spaces after commas, and no spaces in {T<:Ruleset} (I should also make this requirement clear in a PR template)

We could also copy the settings field from the left hand Ruleset, if you feel like doing that too.

Eventually we could define + for Ruleset + SimSettings so that its easy to add settings. But SimSettings hasn't been user-facing up to now and isn't exported. We might want to think about a better name for it, maybe just Settings?

Something like this could be nice:

ruleset = rule1 + rule2 + Settings(boundary=Wrap())

Edit: actually Settings could be different to SimSettings, with default values of nothing so you can just add a single setting and leave the rest unchanged. Or it just holds a NamedTuple.

@gottacatchenall
Copy link
Author

gottacatchenall commented Jul 9, 2021

Eventually we could define + for Ruleset + SimSettings so that its easy to add settings

This would be good and not too hard


We could also copy the settings field from the left hand Ruleset, if you feel like doing that too.

perhaps a getter for Ruleset settings (although only StaticRulesets hold settings at the moment)

settings(rs::T) where {T <: StaticRuleset} = rs.settings

and

Base.:+(a::T, b::V) where {T<:Ruleset,V<:Rule} = Ruleset(rules(a)...,b, settings(a))
Base.:+(a::T, b::V) where {T<:Rule,V<:Ruleset} = Ruleset(a,rules(b)...,, settings(b))
Base.:+(a::T, b::V) where {T<:Ruleset,V<:Ruleset} = Ruleset(rules(a)...,rules(b)..., settings(a))

@rafaqz
Copy link
Member

rafaqz commented Jul 9, 2021

They both do have a settings field, and there is a getter settings(ruleset). So your code example should already work.

We can actually add settings to a ruleset before the simulation, although there was a bug recently where they weren't being used.

For some background on settings and rulesets, there is a little bit of juggling to do with them that complicates things. Settings can be changed with sim! keywords or set at Ruleset definition to allow some flexibility. We have to keep the original Ruleset around as it can be the container for interactive parameter updates, for which it needs to be mutable. But that means we cant pass a Ruleset to a GPU kernel, as its mutable, and we also dont want the compile time of passing in all the rules. So the immutable SimSettings object is what ends up in a GPU kernel, as a field of RuleData.

(I think StaticRuleset is mostly redundant now we have SimSettings).

@rafaqz
Copy link
Member

rafaqz commented Sep 9, 2021

Probably * is more Julian for this, following String concatenation. Rules are also not commutative, like * for matrices.

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.

2 participants