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.

Sign up for GitHub

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

Jump to bottom

Wrapping up GEMINI development #3729

Merged
merged 48 commits into from
Aug 16, 2023
Merged

Wrapping up GEMINI development #3729

merged 48 commits into from
Aug 16, 2023

Conversation

haitamlaarabi
Copy link
Collaborator

  • Integrating Advanced Public SPMC
  • Integrating Ridehail SPMC
  • Updating ScaleUpCharging
  • Updating HELICS deployment
  • Refactoring

@haitamlaarabi
Copy link
Collaborator Author

Test Now!

@haitamlaarabi
Copy link
Collaborator Author

Test now!

JustinPihony
JustinPihony requested changes Feb 21, 2023
View reviewed changes
Copy link
Collaborator

@JustinPihony JustinPihony left a comment

Choose a reason for hiding this comment

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

I was light on the R and Python review. Requesting change because of the missing BeamConfig file

aws/src/main/python/beam_lambda/lambda_function.py
echo "recorder started"
sleep 5s
cd -
path: /home/ubuntu/install-and-run-helics-scripts.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anything call this?

src/main/resources/beam-template.conf Show resolved Hide resolved
src/main/scala/beam/agentsim/infrastructure/ChargingFunctions.scala Outdated Show resolved Hide resolved
@HoneyTauOverTwo
Copy link
Collaborator

test!

@HoneyTauOverTwo
Copy link
Collaborator

@JustinPihony , @haitamlaarabi I am still unsure if the current version of hasValidChargingCapability is correct. The current one seems to imply that an EV cannot, in any circumstance, park somewhere where there is not a charger.

The version present on develop branch will only filter out a parking zone if the inquiry is related to an activity that requires charging.

I suggest adding the verifyCharger check back into it if we are to allow EVs to park somewhere without a charger:

    private def hasValidChargingCapability(zone: ParkingZone, inquiry: ParkingInquiry): Boolean = {
    val verifyCharger = inquiry.beamVehicle.isDefined &&
      inquiry.beamVehicle.get.beamVehicleType.chargingCapability.isDefined && (
        inquiry.searchMode == ParkingSearchMode.EnRouteCharging ||
        inquiry.parkingActivityType == Charge ||
        inquiry.parkingActivityType == EnRoute
      )
    if (!verifyCharger) {
      return true
    }
    zone.chargingPointType match {
      case Some(chargingPointType) =>
        inquiry.beamVehicle.forall {
          case beamVehicle if beamVehicle.isEV =>
            val vehicleHasDCFasChargingCapability =
              beamVehicle.beamVehicleType.chargingCapability.forall(ChargingPointType.isFastCharger)
            val stationHasDCFasChargingCapability = ChargingPointType.isFastCharger(chargingPointType)
            val hasInvalidChargingCapability = !vehicleHasDCFasChargingCapability && stationHasDCFasChargingCapability
            !hasInvalidChargingCapability
          case _ => false
        }
      case _ =>
        false
    }
  }

For some reason, this change fixes the failing test locally beam.integration.EnrouteChargingSpec.Running a single person car-only scenario should do enroute upon not enough charging

I'll push this change and run CI again. @haitamlaarabi, please review the quoted message and let me know if you agree with it.

@HoneyTauOverTwo
Adds back the "verifyCharger" check on "hasValidChargingCapability".
8d5226a 
Without this check, this filter makes that an EV cannot, in any circumstance, park somewhere where there is not a charger.
@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Jul 17, 2023 •
edited
Loading

test!

passed (test getting stuck skipped)

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Jul 17, 2023 •
edited
Loading

test!

(same as before)

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Jul 17, 2023 •
edited
Loading

test!

passed, including the test previously getting stuck. Will run CI another 2 times at least to make sure.

@HoneyTauOverTwo
Copy link
Collaborator

test!

@HoneyTauOverTwo
Modifying DrivingTimeCost test to check if it is getting stuck due to…
75439f6 
… the existence of parking zones without chargers.
@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Jul 31, 2023 •
edited
Loading

test!

CI wasn't working.

1 similar comment
@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Jul 31, 2023 •
edited
Loading

test!

CI wasn't working.

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 2, 2023 •
edited
Loading

test!

This passed, testing again to see it if was a fluke.

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 2, 2023 •
edited
Loading

test!

Failed with test stuck.

@HoneyTauOverTwo
Removes RH vehicles with human drivers from the test getting stuck to…
7396165 
… investigate issue.

Also, reverts the parking file changed on previous commit "Modifying DrivingTimeCost test to check if it is getting stuck due to the existence of parking zones without chargers."
@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

Passed.

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

Still gets stuck without RH with human drivers, so it is most likely something related to L5 RH or both.

@HoneyTauOverTwo
Adds back RH vehicles with human drivers and removes L5 automated veh…
37de3b1 
…icles from the test getting stuck to investigate issue.
@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

fmt failed.

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

Passed.

4 similar comments
@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

Passed.

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

Passed.

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

Passed.

@HoneyTauOverTwo
Copy link
Collaborator

HoneyTauOverTwo commented Aug 3, 2023 •
edited
Loading

test!

Passed.

@HoneyTauOverTwo
Copy link
Collaborator

I was able to reproduce the stuck issue locally as well, and it is definitely related to parallelism.

Also, from the CI results, it seems like the issue is only related to L5 automated RH electric vehicles, so RH electric vehicles with human drivers is not the source of the issue.

@HoneyTauOverTwo
Adds ignore tag to AV RH test getting stuck since issue will be inves…
6c3f79c 
…tigated on another PR (was not originated here)

Also, changes ride hail fleet and the related test to their original state with both automated and non-automated vehicles.
@HoneyTauOverTwo
Copy link
Collaborator

test!

HoneyTauOverTwo
HoneyTauOverTwo approved these changes Aug 15, 2023
View reviewed changes
Copy link
Collaborator

@HoneyTauOverTwo HoneyTauOverTwo left a comment

Choose a reason for hiding this comment

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

Approving since AV RH issue was proven to not be caused by these changes.
This issue will continue to be investigated on #3766.

@HoneyTauOverTwo HoneyTauOverTwo merged commit 08e6d99 into develop Aug 16, 2023
@HoneyTauOverTwo HoneyTauOverTwo deleted the hl/integrating-ridehail-spmc branch August 16, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HoneyTauOverTwo HoneyTauOverTwo HoneyTauOverTwo approved these changes

@JustinPihony JustinPihony JustinPihony approved these changes

@nikolayilyin nikolayilyin Awaiting requested review from nikolayilyin

@dimaopen dimaopen Awaiting requested review from dimaopen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@haitamlaarabi @HoneyTauOverTwo @nikolayilyin @JustinPihony