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

Would be helpful to have a convenience function that returns the time vector #316

Open
moorepants opened this issue Feb 4, 2025 · 7 comments

Comments

@moorepants
Copy link
Member

@property
def time_vector(self):
    return np.arange(0.0, num_nodes*h_val, h_val)
p = Problem(...)
p.time_vector

This can be an attribute as it if very fast to compute. I could be computed on initialization. I guess it should be both on ConstraintCollocator and Problem.

@Peter230655
Copy link
Contributor

Peter230655 commented Feb 4, 2025

@Property
def time_vector(self):
return np.arange(0.0, num_nodes*h_val, h_val)

p = Problem(...)
p.time_vector

This can be an attribute as it if very fast to compute. I could be computed on initialization. I guess it should be both on ConstraintCollocator and Problem.

Would this be useful for both fixed time interval and variable time interval?

Could I include this in my read-only-attributes?
Reason: If you merged this before you merge my read-only-attributes, this will have a conflict again.
To fix these conflicts, at least for me!, takes several hours of very tedious work. :-)

Your suggestion seems to assume that the starting time t0 = 0.0
I have never seen any simulation where this was not the case, except my "three stage" simulation, PR #262.
would it make sense to include an optional keyword t0 = 0.0 to allow for $t_0 \neq 0.0$

@moorepants
Copy link
Member Author

Would this be useful for both fixed time interval and variable time interval?

This would have to output the correct value for fixed and variable. So I guess that means it would need to take the solution as the argument.

Could I include this in my read-only-attributes?

If we need it to work with a variable time interval, I don't' think it can be a property attribute.

Your suggestion seems to assume that the starting time t0 = 0.0

I didn't suggest the precise solution, only an example. We would have to generalize.

@Peter230655
Copy link
Contributor

Would this be useful for both fixed time interval and variable time interval?

This would have to output the correct value for fixed and variable. So I guess that means it would need to take the solution as the argument.

Could I include this in my read-only-attributes?

If we need it to work with a variable time interval, I don't' think it can be a property attribute.

Sorry, what I meant was can I include it in the same PR to avoid conflicts?
(Fixing conflicts with my ``read-only-attributes is a mess!)

Your suggestion seems to assume that the starting time t0 = 0.0

I didn't suggest the precise solution, only an example. We would have to generalize.

@moorepants
Copy link
Member Author

Sorry, what I meant was can I include it in the same PR to avoid conflicts?

No, recommend not mixing topics into PRs. This is "atomically" a different thing.

@Peter230655
Copy link
Contributor

Sorry, what I meant was can I include it in the same PR to avoid conflicts?

No, recommend not mixing topics into PRs. This is "atomically" a different thing.

Fair enough, I understand!
I am on it already.
If possible, and if you want to merge it at all!, merge my read-only-attributes before you merge this.
( I am less worried about my detailed-eoms as you do not seem to like it that much :-) )

@Peter230655
Copy link
Contributor

Peter230655 commented Feb 4, 2025

Question:

Would this require a test function, or is it too simple to be tested?

thanks!

@moorepants
Copy link
Member Author

Would this require a test function, or is it too simple to be tested?

Everything requires a test function.

Peter230655 added a commit to Peter230655/opty that referenced this issue Feb 5, 2025
@moorepants moorepants changed the title Would be helpful to have a convenience funciton that returns the time vector Would be helpful to have a convenience function that returns the time vector Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants