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

feat(mapping): configure type distributions for ScenarioVehicle #333

Merged
merged 8 commits into from
Jul 31, 2023

Conversation

kschrab
Copy link
Contributor

@kschrab kschrab commented Jul 26, 2023

Description

When using predefined SUMO scenarios with, the mapping of applications could be configured using the prototype section by defining a prototype with a name matching the vType identifier of the vehicles, with the weight attribute to control the equipment rate of applications.

However, this allowed only to configure if a vehicle type could be configured with applications or not. No distinction between multiple application deployments could be done. This is now possible with this PR.

In the mapping_config,json, we now allow the typeDistribution section to define a whole distribution of application mappings for one vType identifier from the given SUMO scenario. Let's suppose we only have one vehicle type defined in the SUMO scenario, called "DefaultVehicle", then we can configure a complex application mapping as the following:

"typeDistributions": {
  "DefaultVehicle": [
    { 
      "name": "DefaultVehicle",
      "group": "equippedWithAppA",
      "applications": ["appA"],
      "weight": 0.1 
    },
    { 
      "name": "DefaultVehicle",
      "group": "equippedWithAppB",
      "applications": ["appB"],
      "weight": 0.3 
    },
    { 
      "name": "DefaultVehicle",
      "group": "unequipped",
      "applications": [],
      "weight": 0.6 
    }
  ]
}

Issue(s) related to this PR

  • Resolves issue internal issue 624

Affected parts of the online documentation

Changes in the documentation required?

Yes, LuST tutorial should be adjusted to use typeDistributions instead.

Definition of Done

Prerequisites

  • You have read CONTRIBUTING.md carefully.
  • You have signed the Contributor License Agreement.
  • Your GitHub user id is linked with your Eclipse Account.

Required

  • The title of this merge request follows the scheme type(scope): description (in the style of Conventional Commits)
  • You have assigned a suitable label to this pull request (e.g., enhancement, or bugfix)
  • origin/main has been merged into your Fork.
  • Coding guidelines have been followed (see CONTRIBUTING.md).
  • All checks on GitHub pass.
  • All tests on Jenkins pass.

Requested (can be enforced by maintainers)

  • New functionality is covered by unit tests or integration tests. Code coverage must not decrease.
  • If a bug has been fixed, a new unit test has been written (beforehand) to prove misbehavior
  • There are no new SpotBugs warnings.

Special notes to reviewer

allows defining multiple application types for equal vehicle type names in large sumo scenario

Signed-off-by: Karl Schrab <[email protected]>
@kschrab kschrab added the enhancement New feature or request label Jul 26, 2023
@kschrab kschrab requested a review from schwepmo July 26, 2023 07:45
@kschrab kschrab marked this pull request as ready for review July 26, 2023 07:46
@schwepmo
Copy link
Contributor

schwepmo commented Jul 27, 2023

Copy link
Contributor

@schwepmo schwepmo left a comment

Choose a reason for hiding this comment

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

Some minor remarks but overall looks fine

return;
final List<CPrototype> typeDistribution = framework.getTypeDistributionByName(scenarioVehicle.getVehicleType().getName());
if (!typeDistribution.isEmpty()) {
final CPrototype selected = new StochasticSelector<>(typeDistribution, randomNumberGenerator).nextItem();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to somehow re-use the StochasticSelector for each separate type distribution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do this somehow I guess, but I'm afraid that it would blow up the code here and would decrease readability. I'm not sure if this would even improve anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the BeST-scenario this would cause ~3 million unnecessary initializations of a StochasticSelector, wouldn't it? Also I don't know how the randomness is affected if we draw a type distribution from the same selector or newly initialized ones, although I would assume this doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this "cache".

Previous:

final CPrototype selected = new StochasticSelector<>(typeDistribution, randomNumberGenerator).nextItem();

Now:

StochasticSelector<CPrototype> selector = typeDistributionSelectors.get(scenarioVehicle.getVehicleType().getName());
if (selector == null) {
    selector = new StochasticSelector<>(typeDistribution, randomNumberGenerator);
    typeDistributionSelectors.put(scenarioVehicle.getVehicleType().getName(), selector);
}
final CPrototype selected  = selector.nextItem();

Alternative approach, but I'm not 100% sure if other objects (e.g. the object for the lambda) are created instead, that's why I ditched this for now:

final CPrototype selected  = typeDistributionSelectors.computeIfAbsent(
    scenarioVehicle.getVehicleType().getName(), (v) -> new StochasticSelector<>(typeDistribution, randomNumberGenerator)
).nextItem();

@kschrab
Copy link
Contributor Author

kschrab commented Jul 27, 2023

I think we will need to adjust some further parts of the documentation:

Yes, thanks for pointing out the parts for the adjustments.

@kschrab
Copy link
Contributor Author

kschrab commented Jul 31, 2023

@schwepmo please review again, I used the typeDistribution field in the Sievekingplatz scenario and added an intergration test for it

@kschrab kschrab requested a review from schwepmo July 31, 2023 09:26
@schwepmo schwepmo merged commit 750a77a into main Jul 31, 2023
1 check passed
@kschrab kschrab deleted the 624-scenario-vehicle-type-dist branch July 31, 2023 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants