Correct crank angle and make inclusion of rider inertia in rear frame optional

[josefariasfuentes]

No description provided.

[tjstienstra]

The mistake in the crank angle is a good catch.

It would indeed be nice to make it easier for a user to choose whether the inertial properties of the human should be include in the rear frame. However, I would prefer a slightly different implementation.

Two immediate solutions for this feature sprang to mind:

• Making it possible to pass a keyword argument to get_param_values. This will require one to call the rear frame separately to overwrite the obtained values, e.g. bicycle_rider.get_param_values(bicycle_params) | bicycle_rider.bicycle.rear_frame.get_param_values(bicycle_params, include_rider_inertia=False). The advantage of this method is that it keeps things strongly tied to the get_param_values method and it doesn't clutter the attribute interface.
• Making an attribute include_rider_intertia, which is set to a default value in __init__. There are two changes w.r.t. your current implementation. Namely, the usage of an attribute instead of a class variable combined with a property.

As for the possible values of include_rider_inertia:

• True: Include the rider inertia in the rear frame inertia. These will be the benchmark parameters if no rider is specified. However, if a rider is specified then a different computation should be used. If the bicycle_parameters package does not offer an easy option for this, then we should raise a NotImplemntedError.
• False: Exclude the rider inertia in the rear frame inertia. This can be done by recomputing the benchmark parameters from the bicycle as is currently also done when the bicycle parameters have a rider.
• None: Choose True if the bicycle parameters don't specify a human else choose False. The advantage of this option would be that we can maintain backward compatibility.

Reflecting on these options, @josefariasfuentes which solution would have your preference? Mine would probably be the first.

[josefariasfuentes]

Hi TImo, good point. I prefer the first one as well. Thanks and best regards

…

__________________________________________________________ José Alfonso Farías Fuentes PhD student at the Department of Industrial and Information Engineering Università degli Studi di Pavia Via A. Ferrata, 5 27100 Pavia Il giorno gio 13 mar 2025 alle ore 16:51 Timo Stienstra < ***@***.***> ha scritto:

The mistake in the crank angle is a good catch. It would indeed be nice to make it easier for a user to choose whether the inertial properties of the human should be include in the rear frame. However, I would prefer a slightly different implementation. Two immediate solutions for this problem sprang to mind: - Making it possible to pass a keyword argument to get_param_values. This will require one to call the rear frame separately to overwrite the obtained values, e.g. bicycle_rider.get_param_values(bicycle_params) | bicycle_rider.bicycle.rear_frame.get_param_values(bicycle_params, include_rider_inertia=False). The advantage of this method is that it keeps things strongly tied to the get_param_values method and it doesn't clutter the attribute interface. - Making an attribute include_rider_intertia, which is set to a default value in __init__. There are two changes w.r.t. your current implementation. Namely, the usage of an attribute instead of a class variable combined with a property. As for the possible values of include_rider_inertia: - True: Include the rider inertia in the rear frame inertia. These will be the benchmark parameters if no rider is specified. However, if a rider is specified then a different computation should be used. If the bicycle_parameters package does not offer an easy option for this, then we should raise a NotImplemntedError. - False: Exclude the rider inertia in the rear frame inertia. This can be done by recomputing the benchmark parameters from the bicycle as is currently also done when the bicycle parameters have a rider. - None: Choose True if the bicycle parameters don't specify a human else choose False. The advantage of this option would be that we can maintain backward compatibility. Reflecting on these options, @josefariasfuentes <https://github.com/josefariasfuentes> which solution would have your preference? Mine would probably be the first. — Reply to this email directly, view it on GitHub <#149 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BJSGZ6JVN7IPFFSE2D4SAXL2UGSPNAVCNFSM6AAAAABY6PCJY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMRRG42DCMRRGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***> [image: tjstienstra]*tjstienstra* left a comment (mechmotum/symbrim#149) <#149 (comment)> The mistake in the crank angle is a good catch. It would indeed be nice to make it easier for a user to choose whether the inertial properties of the human should be include in the rear frame. However, I would prefer a slightly different implementation. Two immediate solutions for this problem sprang to mind: - Making it possible to pass a keyword argument to get_param_values. This will require one to call the rear frame separately to overwrite the obtained values, e.g. bicycle_rider.get_param_values(bicycle_params) | bicycle_rider.bicycle.rear_frame.get_param_values(bicycle_params, include_rider_inertia=False). The advantage of this method is that it keeps things strongly tied to the get_param_values method and it doesn't clutter the attribute interface. - Making an attribute include_rider_intertia, which is set to a default value in __init__. There are two changes w.r.t. your current implementation. Namely, the usage of an attribute instead of a class variable combined with a property. As for the possible values of include_rider_inertia: - True: Include the rider inertia in the rear frame inertia. These will be the benchmark parameters if no rider is specified. However, if a rider is specified then a different computation should be used. If the bicycle_parameters package does not offer an easy option for this, then we should raise a NotImplemntedError. - False: Exclude the rider inertia in the rear frame inertia. This can be done by recomputing the benchmark parameters from the bicycle as is currently also done when the bicycle parameters have a rider. - None: Choose True if the bicycle parameters don't specify a human else choose False. The advantage of this option would be that we can maintain backward compatibility. Reflecting on these options, @josefariasfuentes <https://github.com/josefariasfuentes> which solution would have your preference? Mine would probably be the first. — Reply to this email directly, view it on GitHub <#149 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BJSGZ6JVN7IPFFSE2D4SAXL2UGSPNAVCNFSM6AAAAABY6PCJY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMRRG42DCMRRGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[josefariasfuentes]

Hi Timo, I think to include the first option that we talk last month here. Please let me know if this solution was what you think. It would be nice to have this feature merged with the main branche since I want to run some simulations in other pcs importing symbrim with conda - forge and then run the codes without modifying the src code. Thanks in advance

[tjstienstra]

Oops, saw that I had started a review, but didn't finish it. Here is at leat some initial feedback.
Two points that I should still note are:

• None and False always give the same result, currently. If we do support None, then it should also be the default and in that case the easiest option is probably to start the function with an is None check and converting it to a boolean based on a condition.
• You will probably also be required to add a simple unittest.

[tjstienstra]

Could you reformat this docstring according to the numpy docstring format. Doing so you will be adding a parameters section, where you can put a simple explanation on what include_rider_inertia does, refer to symbrim/bicycle/grounds.py for two examples.

[tjstienstra]

    def get_param_values(
        self,
        bicycle_parameters: Bicycle,
        include_rider_inertia: bool | None = False,
    ) -> dict[Symbol, float]:

As the online checks are also pointing out, there are some formatting issues.