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

Fix time simulations #16

Closed
jondea opened this issue May 17, 2018 · 4 comments
Closed

Fix time simulations #16

jondea opened this issue May 17, 2018 · 4 comments
Milestone

Comments

@jondea
Copy link
Member

jondea commented May 17, 2018

Some thought needs to go into the outer structures for the TimeSimulations again we can obviously reuse a lot of the code which does the mathsy heavy lifting, but we need some motivating examples for the structs. We also need to think about how the TimeSimulations will interact with the random machinery.

@arturgower
Copy link
Member

arturgower commented Jun 2, 2018

I am unsure if we need a struct TimeSimulations. It is nice to have a struct where the impulse and source functions are in time. Drawbacks and a positive are:

  • If gives the imprecision that are code does some sort of simulation in time, rather than doing a simulation in frequency and then converting to time. This difference is important to know even for a user generating some data.
  • Given something like run(TimeSimulation, t_vec), thought/code will be needed to decide how to sample ω_vec, though we can just force the user to pass ω_vec. For instance, what does run(TimeSimulation, [t]) do?
  • Both FrequencySimulation and TimeSimulation represent almost exactly the same information (with the exception of the source function). This mean that using f(FrequencySimulation; time=true, source = default) could replace all instances of f(TimeSimulation).
  • On the positive side, having another struct makes for nice dispatch, for example, plot(TimeSimulation, t).

@arturgower
Copy link
Member

Have implemented my first suggestion: run(FrequencySimulation, x_vec; ts = time_vec) or run(FrequencySimulation, x_vec, ω_vec; result_in_time = true) now returns a TimeSimulationResult. Having something like this would be very practical.

I've also moved the zero frequency response extrapolation to run, which turned out to be simpler and make more sense (then having it in the signal processing part).

@jondea
Copy link
Member Author

jondea commented Jun 4, 2018

I think it is confusing that run(FrequencySimulation,...) sometimes returns a TimeSimulationResult, but I agree the function looks useful. I think a different name would be much clearer, something which communicates run_and_convert_to_time.

We can always have TimeSimulationResult as an optional structure which contains a FrequencySimulation which essentially calls this function.

@arturgower
Copy link
Member

I think having different names for the run function, such as run_and_convert... would hurt discoverability, and is not very Julia. If the user is unaware of the type system, they won't mind having a TimeSimulationResult returned to them, as it will be used in exactly the same way as FrequencySimulationResult, i.e. plot(TimeSimulationResult) and TimeSimulationResult.field.

Alternatively, if the user is aware of the type system, I think they will quickly figure out that specifying a time array, such as run(FrequencySimulation, x_vec; ts = time_vec). There is also the function frequency_to_time and time_to_frequency to easily convert between the types.

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