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

Allow bank_index in initializers #1101

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Conversation

edwardalee
Copy link
Collaborator

A common problem with banks is that members of the bank may need to each have its own set of parameter values. The only mechanism we currently offer requires modifying the reactor class itself, which needs to set state variables (rather than parameters) from a global table. This means that the reactor can only be used in a bank and must be written to work in bank.

This PR adds to the C and Python targets the ability to initialize parameters using a table lookup and the bank_index variable. Here is an example:

target C;
preamble {=
    int table[] = {4, 3, 2, 1};
=}
reactor Source(bank_index:int(0), value:int(0)) {
    output out:int;
    reaction (startup) -> out {=
        SET(out, self->value);
    =}
}
main reactor {
    source = new[4] Source(value = {= table[bank_index] =});
}

The table defined in the preamble determines the parameter values to use for each bank member.

This change was motivated by @Soroosh129's PacMan game example, where I have made the ghosts into a bank rather than four individual reactor instances.

@edwardalee edwardalee requested review from Soroosh129 and cmnrd April 18, 2022 21:27
Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

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

This is a neat idea. It's very useful. In the past, we had to resort to lambda functions, such as here.

To clarify, I made ghosts into individual instances instead of members of a bank based on the comment that each ghost should have its own personality. The idea was then to subsequently create a reactor class for each ghost.

Alternatively, we could pursue the idea of enabling reactor classes to be passed as parameters. Then, the same Ghost reactor could be used with a parametrized Behavior reactor.

Or, there might be a third approach to creating unique behaviors for each ghost, while still enjoying the compactness that banks bring to the table.

@cmnrd
Copy link
Collaborator

cmnrd commented Apr 19, 2022

This is a great idea and I like that it is very general. Instead of a table lookup, we could also invoke a function and pass the bank index as argument for some computation.

We should also add this for the other targets. I am not sure what would be best: open an issue to not forget about this or attempt at adding support for all targets in this PR?

@edwardalee edwardalee merged commit e1b40d7 into master Apr 19, 2022
@edwardalee edwardalee deleted the bank-index-in-initializers branch April 19, 2022 15:04
@lhstrh lhstrh changed the title Allow bank_index in initializers. Allow bank_index in initializers May 2, 2022
@lhstrh lhstrh added the c Related to C target label May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c Related to C target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants