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

WIP Add Dimension Key Access to run_timestep function #1017

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented Oct 23, 2024

Handling part of #948 (also mentions access to t in init but save that for a second PR)

The goal of this PR is to add access to the dimension keys from within components. Currently for a run_timestep(p,v,d,t) function, d.dim_name will return a vector of the Type needed to index into Arrays for the dimension, namely Ints for all dimensions except time for which it is our custom Timestep type.

While this is useful for a loop like

for d in d.countries
    v.var1[d] = p.par1 * p.mulitplier[d]
end

there are times a user would like to check the actual keys, I think the core example being something like

for d in d.countries
    if d == "USA" # FAILS
        # do something
    else
        # do something else
    end
end

Note the line above fails because d is an Integer (eg. in GIVE we know it is 174, the index of the key "USA" in the countries dimension vector).

One way to pass information to run_timestep without breaking compatibility is to have keyword args we set during initialization, and then are in scope for the function, that the user never needs to see or manipulate. We have access to the entire ModelInstance at the time of running run_timestep.

Some decisions/TODOs

  1. What should we pass? On my first try I pass a get_dim_keys(dim_name::Symbol) function, but we could really pass any structure that is useful. Instead of a function we could have an object called d_keys that is exactly the same as d but returns the keys?
  2. Add this to init as well (?)
  3. Think carefully about the time dimension keys here -- there can be offsets etc.
  4. Add tests

@lrennels
Copy link
Collaborator Author

lrennels commented Oct 23, 2024

@jrising @djsmithumn @bryanparthum think you've all mentioned this to me -- open to ideas here as I consider options -- will talk to @davidanthoff as well.

@jrising now that I see this implementation I admit we could pretty easily embed a function like this_model() like you do, or something generic like get_metadata() that we add to, but also trying to keep it fairly simple for now.

@lrennels lrennels changed the title Add Dimension Key Access to run_timestep function WIP Add Dimension Key Access to run_timestep function Oct 23, 2024
Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.83%. Comparing base (9da9ce9) to head (a9e9528).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
- Coverage   84.84%   84.83%   -0.01%     
==========================================
  Files          40       40              
  Lines        4070     4069       -1     
==========================================
- Hits         3453     3452       -1     
  Misses        617      617              
Flag Coverage Δ
unittests 84.83% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@lrennels
Copy link
Collaborator Author

Current implementation (not totally happy with it though ...) would let you do ...

for d in d.countries
    if get_dim_keys(:countries)[d] == "USA"
        # do something
    else
       # do something else
    end
end

we could also have something like

for d in d.countries
    if d_keys.countries[d] == "USA"
        # do something
    else
       # do something else
    end
end

or maybe something fancier ...

@jrising
Copy link
Contributor

jrising commented Oct 24, 2024

The get_dim_keys pseudo-function looks good to me. I guess I would just suggest that in the unit test, you spin up two different models with the same component but different dimension values, to sanity-check that there are no global dependencies in the get_dim_keys function.

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