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 model.calibration[:states] return a SVector ? #124

Open
albop opened this issue Dec 9, 2017 · 1 comment
Open

Should model.calibration[:states] return a SVector ? #124

albop opened this issue Dec 9, 2017 · 1 comment

Comments

@albop
Copy link
Member

albop commented Dec 9, 2017

Currently the model.calibration object returns a Vector when indexed with a (many) Symbol (s).
At making it return an SVector would sound like a no brainer, but it raises many complications:

  • currently all the user-facing facing APIs take/return regular vectors. For instance in commonsimulate(model,dr,s0), s0 is a vector. Since the most natural way to get this s0 is to do s0 = model.calibration[:states], we would need to accept an SVector instead in addition to a Vector.
  • these user-facing functions should definitely accept both kind of inputs. We could just use AbstractArray, but we also want type inference to work on size, so there might be a trade-off there. Essentially we need to find the right way to do the casting here and use it everywhere. I'll open a separate issue for that.
  • if you want to start off-equilibrium, you currently do s0 = model.calibration[:states] and for instance s0[1]*=1.01. This wouldn't work with a static vector. We could consider the mutable version MVector but I truly doubt it is worth the trouble.
    (maybe s0[1]*=1.01 could be replaced by s0 = s0 + e[1] where e would be some kind of magical coordinate vector whose dimension would automatically adjust when added to another vector).

Overall I'm inclined to not change what model.calibration returns, or if we really want, introduce some kind of strange looking model.calibration[Point,:states]. But I'm open to discussion. So much so I'm actually opening one.
@sglyon ?

@sglyon
Copy link
Member

sglyon commented Dec 12, 2017

I think that we should always strive for simplicity in the public facing api. If having the calibration objects returns SVectors instead of Vectors complicates that, we should probably avoid it.

That isn't to say there isn't a clever workaround like what you mention at the end. If anything comes to mind I'll let you know!

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

No branches or pull requests

2 participants