added version of bridge that includes velocities of individual stars …

[webbjj]

Note that the changes to bridge itself are minor. It just needs a use_velocities flag and self.vx,self.vy,self.vz need to be passed in get_gravity_at_point. I don't think they are needed in other functions. So it may not be necessary to have a separate file, and bridgev could be folded into bridge.

[rieder]

I would definitely fold it into bridge.py, and make it optional to pass the velocities. It would also make the changes easier to spot for us :).

I think the use_velocities flag should be passed when adding a "kicker" system, individually for each of these. Not all system may support get_gravity_at_point with velocities.

[ipelupessy]

great! I would also put the changes in new (derived) classes in this case (and a more descriptive name than bridgev) in bridge.py. Can you also add an example of its use and some tests wouldalso be nice?

also next time: i suggest you make a feature branch in your fork and a PR against this (this makes it easier for yourself to work on multiple things at the same time;-)...)

[webbjj]

I would definitely fold it into bridge.py, and make it optional to pass the velocities. It would also make the changes easier to spot for us :).

I think the use_velocities flag should be passed when adding a "kicker" system, individually for each of these. Not all system may support get_gravity_at_point with velocities.

Where is the check to see if the system is a "kicker" or not? Its not entirely clear to me where this is in bridge.

[webbjj]

great! I would also put the changes in new (derived) classes in this case (and a more descriptive name than bridgev) in bridge.py. Can you also add an example of its use and some tests wouldalso be nice?

also next time: i suggest you make a feature branch in your fork and a PR against this (this makes it easier for yourself to work on multiple things at the same time;-)...)

Thanks for the suggestion. I am still a bit of a git rookie.

I was also hoping you can elaborate on your initial suggestion to put the changes in new (derived) classes. Are you saying make a class Bridgev (with better name) as opposed to adding the use_velocities option to class Bridge? Similarly, add a class GravityCodeInFieldv instead of adding to GravityCodeInField?

[ipelupessy]

ok, I checked more carefully now: you should indeed make a new:

class BridgeWithVelocityDependendForces(Bridge):
..etc

(or some other descriptive name)..the option use_velocity still has to be present because as it is now you can enable these on a per system basis.

eventually this could be folded back in the Bridge, but I think it is nice to have a new class for now since that makes it clearer what the differences are.

already mentioned this, but tests and a way to validate would also be nice;-)

[rieder]

I made some changes in a PR to this branch, creating a separate VelocityDependentBridge class (which mostly just inherits from Bridge) and merging the other changes into bridge.py.

[rieder]

I'm not sure if I like the re-defining of "get_gravity_at_point" with velocities.

Perhaps we should have a new function "get_force_at_point" for this purpose - after all, the velocity dependent forces are different from gravity...

[ipelupessy]

one thing I don't fully like: the method to get the acceleration in the velocity kick case is still called get_gravity; maybe it should be renamed to get_acceleration?

[ipelupessy]

ok, I see you had the same concern ;-)

[rieder]

@ipelupessy should we perhaps just move to get_acceleration_at_point altogether, leaving get_gravity_at_point as an alias for this in case it is different?

[rieder]

Reminder to self that this is close to being mergeable. Just needs a name change for some functions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[ipelupessy]

keep open for now

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 28 days if no further activity occurs. Thank you for your contributions.

[ipelupessy]

keep

[rieder]

This is still worth merging. Needs only a little bit of updating.