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

Optimize dimension order? #388

Closed
rjplevin opened this issue Jan 29, 2019 · 13 comments

Comments

@rjplevin
Copy link
Collaborator

commented Jan 29, 2019

Currently we require time to be the first dimension, if it occurs. There are two issues with this:

  1. Users' code must conform to this (there is a PR for an error message if not in right order)
  2. We might get better performance with time as the last dimension

Look into mods to our setindex and getindex functions to see if we can re-order easily behind the scenes, also allowing the user to specify dimensions however they like.

@rjplevin rjplevin self-assigned this Jan 29, 2019

@lrennels lrennels added this to the v0.7.0 milestone Feb 2, 2019

@lrennels lrennels added the to discuss label Feb 2, 2019

@davidanthoff davidanthoff modified the milestones: v0.7.0, Backlog Feb 15, 2019

@jrising

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

I was open another issue on this front. Relaxing this requirement is quite a pressing need for updating my model, which is designed entirely with time as the last dimension.

@davidanthoff davidanthoff modified the milestones: Backlog, Triage Mar 8, 2019

@lrennels lrennels assigned ckingdon95 and unassigned rjplevin Mar 8, 2019

@lrennels

This comment has been minimized.

Copy link
Collaborator

commented Mar 8, 2019

@jrising we discussed this today, @ckingdon95 will look at how much of a lift this will be. We know it's pressing so we'll look ASAP!

@jrising

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

Thanks for looking into this. I definitely appreciate it.

@ckingdon95

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2019

@jrising I'm looking at this now! One quick questions as I'm starting to try this out: In your model where time is the last dimension, is this reflected in all of your @defcomps and run_timestep functions? i.e. parameters are defined as Parameter(index=[r, time]) and then during run timestep they are referenced as p.paramname[r, ts] accordingly?

@jrising

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I'm not sure what you're asking. The alternatives to saying p.param[r, t] that I can think of are to say p.param[q, r, s, t] (when there are more dimensions), or saying things like p.param[q, :, :, t] (really all combinations of colon operators in all dimensions except for the time dimension).

@ckingdon95

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

Sorry yes my question wasn't very clear. I was trying to make sure I was understanding the functionality you want, which is to be able to place the time index in the last (or any position) in each of those two places-- in the initial definition Parameter(index=[regions, somethingelse, time]) and then also in the run_timestep access with p.param[r, s, t]-- and that the positioning must be the same between the two places (but doesn't have to be the same for all parameters and variables). I can't imagine a different way of doing it, but just wanted to make sure that's what you're doing!

@ckingdon95

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

@jrising I have an implementation that seems to be working, and I opened a pull request here: #434

I'm going to be cleaning it up and adding more tests, but I think the best test would be for you to try to run your model off of the branch "time" whenever you have the chance!

@jrising

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Awesome! I looked through your PR, and it seems general (with your split_indices approach) and the fact that you changed the type should mean that you got all the necessary changes. It will take me at least a week to test things though in my model; right now I'm running up against a versioning problem with GlobalSensitivityAnalysis. But I'm hopeful!

@lrennels

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

@jrising what is the versioning problem you're getting?

@jrising

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

It turned out to be a versioning error with trying to use julia 0.7 again. It went away in Julia 1.*. I'm still testing things.

@lrennels lrennels modified the milestones: Triage, v0.10.0 May 14, 2019

@jrising

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Okay! Everything has checked out. Thanks for this.

@ckingdon95

This comment has been minimized.

Copy link
Collaborator

commented Jun 17, 2019

Wonderful! We will merge this into master soon.

@lrennels

This comment has been minimized.

Copy link
Collaborator

commented Jun 25, 2019

Done and merged to master in #434!

@lrennels lrennels closed this Jun 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.