Skip to content

Remove camelCase properties#1440

Merged
Ozaq merged 1 commit intoPedestrianDynamics:masterfrom
chraibi:renaming_arguments
Mar 16, 2025
Merged

Remove camelCase properties#1440
Ozaq merged 1 commit intoPedestrianDynamics:masterfrom
chraibi:renaming_arguments

Conversation

@chraibi
Copy link
Copy Markdown
Contributor

@chraibi chraibi commented Jan 19, 2025

This PR addresses issue #1415.

  • Introduce new properties.
  • Mark existing camelCase properties as deprecated with appropriate warnings
  • Verify the functionality of the new snake_case properties.
  • Test deprecated camelCase properties to ensure they still work and raise deprecation warnings.
  • Ensure consistency between the old and new properties (changes to one are reflected in the other).
  • Include a Ruff check to enforce snake_case naming patterns while explicitly ignoring violations for deprecated camelCase properties.
  • Rename v0 in all models to desired_speed

@chraibi chraibi changed the title Add tests for deprecated camelCase properties of SFM Remove camelCase properties from SFM Jan 19, 2025
@Ozaq Ozaq added this to the v1.3.0 milestone Jan 22, 2025
@chraibi chraibi changed the title Remove camelCase properties from SFM Remove camelCase properties Jan 31, 2025
@chraibi chraibi marked this pull request as ready for review January 31, 2025 16:55
@chraibi chraibi requested a review from Ozaq February 1, 2025 05:38
Comment on lines +106 to +109
PyErr_WarnEx(
PyExc_DeprecationWarning,
"'v0' is deprecated, use 'desired_speed' instead.",
1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just rename the property, this is an internal only interface, this can be changed freely.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same is true for all changes in this PR affecting the bindings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added in test_desired_speed another test:

  • test deprecated params when adding agents (works)
  • test deprecated params after adding agents (does not work, unless I touch the internal cpp interface)

Comment on lines +101 to +111
"""Init dataclass to handle deprecated argument."""
if v0 is not None:
warnings.warn(
"'v0' is deprecated, use 'desired_speed' instead.",
DeprecationWarning,
stacklevel=2,
)
self.desired_speed = v0

self.__dict__.update(kwargs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AVM model has not yet been released, you can freely modify the names still. No need to release with any deprecation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I remove v0 from avm.

def __init__(
self,
v0=None,
**kwargs,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope we cannot do it like this, now unknown parameters are silently ignored, e.g.
CollisionFreeSpeedModelAgentParameters(foo=True) was an error and is now silently ignored.

import warnings
from dataclasses import dataclass

from deprecated import deprecated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from deprecated import deprecated
import sys
if sys.version_info < (3, 13):
from deprecated import deprecated
else:
from warnings import deprecated

@deprecated decorators got added with 3.13, see https://docs.python.org/3/library/warnings.html#warnings.deprecated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply the same in all places where you imported deprecated

Comment thread systemtest/test_desired_speed.py Outdated
@@ -0,0 +1,74 @@
# Copyright © 2012-2024 Forschungszentrum Jülich GmbH
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Copyright © 2012-2024 Forschungszentrum Jülich GmbH

@Ozaq Ozaq force-pushed the renaming_arguments branch 2 times, most recently from 9e88a28 to 4a89844 Compare March 2, 2025 18:48
Comment thread python_modules/jupedsim/jupedsim/models/collision_free_speed_v2.py
Comment on lines +88 to +95
# if v0 is not None:
# warnings.warn(
# "'v0' is deprecated, use 'desired_speed' instead.",
# DeprecationWarning,
# stacklevel=2,
# )
# self.desired_speed = v0
# else:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check if v0 and desired_speed are set by the user

Comment thread python_modules/jupedsim/jupedsim/models/social_force.py
Comment on lines +123 to +143
deprecated_map = {
"desiredSpeed": "desired_speed",
"reactionTime": "reaction_time",
"agentScale": "agent_scale",
"obstacleScale": "obstacle_scale",
"forceDistance": "force_distance",
}
for old_name, new_name in deprecated_map.items():
if old_name in locals() and locals()[old_name] is not None:
warnings.warn(
f"'{old_name}' is deprecated, use '{new_name}' instead.",
DeprecationWarning,
stacklevel=2,
)
setattr(self, new_name, locals()[old_name])

self.desired_speed = desired_speed
self.reaction_time = reaction_time
self.agent_scale = agent_scale
self.obstacle_scale = obstacle_scale
self.force_distance = force_distance
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check if new_name and old_name are set by the user

@Ozaq Ozaq force-pushed the renaming_arguments branch from bfbf257 to 94e3a67 Compare March 16, 2025 16:06
@Ozaq Ozaq self-requested a review March 16, 2025 16:11
Co-authored-by: Ozaq <ozaq@posteo.de>
Co-authored-by: schroedtert <t.schroedter@posteo.de>
@Ozaq Ozaq force-pushed the renaming_arguments branch from 94e3a67 to df640fc Compare March 16, 2025 16:18
@Ozaq Ozaq merged commit 81e13ee into PedestrianDynamics:master Mar 16, 2025
44 checks passed
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

Successfully merging this pull request may close these issues.

3 participants