Cjrs/#262 parking 4ci #384

colinsheppard · 2018-08-12T03:47:51Z

Parking tests are passing. I haven't investigated whether the rest of the test suite is passing, but they can be followed here:

https://beam-ci.tk/blue/organizations/jenkins/beam-4ci/detail/beam-4ci/779/pipeline

closes #262

This change is

…sion

…nsit

…rst to start of last) and MATSim/BEAM (end of first to end of last)

…mented

…person to manager

…nMeters parameter added to config

…hoiceInMeters

values

… into fdariasm/parking

sfwatergit

Generally ok. See comments. We should focus on improved commenting throughout our code if we are to begin effectively opening this project up to outside developers.

Reviewed 121 of 121 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @colinsheppard and @fdariasm)

src/main/scala/beam/agentsim/Resource.scala, line 61 at r1 (raw file):

    * @tparam T Any ID type
    */
  def checkInResource[T](whenWhere: Option[SpaceTime], executionContext: ExecutionContext)(

The excecutionContext can be passed in as an implicit parameter. Don't worry about it though. I'll fix later.

src/main/scala/beam/agentsim/agents/PersonAgent.scala, line 145 at r1 (raw file):

    ): DrivingData = copy(currentLegPassengerScheduleIndex = currentLegPassengerScheduleIndex)

    override def hasParkingBehaviors: Boolean = true

What's the purpose of this? Can it be inferred from other available data or traits on the PersonAgent?

src/main/scala/beam/agentsim/agents/PersonAgent.scala, line 445 at r1 (raw file):

   * now complete. There are four outcomes possible:
   *
   * 1 There are more legs in the trip and the PersonAgent is the driver => goto WaitingToDrive and schedule

Let's start following ScalaDoc conventions... please use [[<class_name>> <<(optional) text name>>]] in comments when referring to classes. Later we can fold this into the documentation, but more importantly, we can use automatic refactoring to ensure that class names migrate in comments as well as code.

src/main/scala/beam/agentsim/agents/Population.scala, line 51 at r1 (raw file):

  private implicit val timeout: Timeout = Timeout(50000, TimeUnit.SECONDS)

  var initParkingVeh: Seq[ActorRef] = Nil

If this is meant to be a private collection, then prefer use of private val ... mutable.<<CollectionClass>>. It's a sort of optimization.

src/main/scala/beam/agentsim/infrastructure/ParkingManager.scala, line 24 at r1 (raw file):

    destinationUtm: Location,
    activityType: String,
    valueOfTime: Double,

Why should activityType and valueOfTime and other agent-specific queries be included in the inquiry? Shouldn't these be

src/main/scala/beam/agentsim/infrastructure/ParkingStall.scala, line 21 at r1 (raw file):

object ParkingStall {
  case class StallAttributes(

Some of these data should be specific to the manager in order to avoid replicating them for each stall.

src/main/scala/beam/agentsim/infrastructure/TAZTreeMap.scala, line 212 at r1 (raw file):

    val features: util.Collection[SimpleFeature] = shapeFileReader.getFeatureSet

    lazy val utm2Wgs: GeotoolsTransformation = new GeotoolsTransformation("EPSG:4326", "EPSG:26910")

There is a class, WGSConverter in PlanSamplerApp that we may wish to use in order to avoid repeating this code everywhere. Also, source datum should be read from the shapefile.

src/main/scala/beam/agentsim/infrastructure/ZonalParkingManager.scala, line 99 at r1 (raw file):

    case inquiry @ DepotParkingInquiry(location: Location, reservedFor: ReservedParkingType) =>
      val mNearestTaz = findTAZsWithDistances(location, 1000.0).headOption

Avoid mixing in new naming conventions...mNearest... is an example of a Java naming convention that I only see here. If we plan to use something like this, let's add to style guide (TODO).

src/main/scala/beam/agentsim/infrastructure/ZonalParkingManager.scala, line 238 at r1 (raw file):

  }

  def selectPublicStall(inquiry: ParkingInquiry, searchRadius: Double): ParkingStall = {

Perhaps we should start an issue, but I think it would make sense to ensure all utility computations are local to agents. H

src/main/scala/beam/agentsim/infrastructure/ZonalParkingManager.scala, line 316 at r1 (raw file):

  def selectStallWithCharger(inquiry: ParkingInquiry, startRadius: Double): ParkingStall = ???

  def readCsvFile(filePath: String): mutable.Map[StallAttributes, StallValues] = {

Let's derive a generalized CSV reader class. If it's trivial, then at least make this method name more specific (e.g., readParkingCsvFile) or else make it private to this class.

src/main/scala/beam/sim/BeamHelper.scala, line 1 at r1 (raw file):

package beam.sim

Minor, could change class name to BeamSimHelper as that is more descriptive. Also, I have some ideas on how to clean this up, since it's getting pretty hairy at this point, but knowing its contents is a key component of simulation configuration.

src/main/scala/beam/utils/CsvUtils.scala, line 12 at r1 (raw file):

object CsvUtils {

MATSim already has this kind of utility in IOUtils... it is much more robust. Let's just use that.

src/main/scala/beam/utils/CsvUtils.scala, line 23 at r1 (raw file):

  }

  def getHash(concatParams: Any*): Int = {

Move into FileUtils

src/test/scala/beam/agentsim/agents/ridehail/RideHailSurgePricingManagerSpec.scala, line 150 at r1 (raw file):

      val beamConfig: BeamConfig = BeamConfig(config)
      val treeMap: TAZTreeMap = getTazTreeMap(beamConfig.beam.agentsim.taz.file)
      val rhspm = new RideHailSurgePricingManager(beamConfig, Some(treeMap))

rename to surgePricingManager

src/test/scala/beam/integration/ParkingSpec.scala, line 63 at r1 (raw file):

        "beam.outputs.events.overrideWritingLevels",
        ConfigValueFactory.fromAnyRef(
          "beam.agentsim.events.ParkEvent:VERBOSE, beam.agentsim.events.LeavingParkingEvent:VERBOSE, org.matsim.api.core.v01.events.ActivityEndEvent:REGULAR, org.matsim.api.core.v01.events.ActivityStartEvent:REGULAR, org.matsim.api.core.v01.events.PersonEntersVehicleEvent:REGULAR, org.matsim.api.core.v01.events.PersonLeavesVehicleEvent:REGULAR, beam.agentsim.events.ModeChoiceEvent:VERBOSE, beam.agentsim.events.PathTraversalEvent:VERBOSE"

These are hard to read and may result in brittle tests. It may make sense to start externalizing test resources into their own directory. In particular, whenever the beam.conf is changed for beamville or sf-light and tests depend on these configs, we will end up breaking tests more often than not. Even if we have to duplicate some files, I think it makes sense to have a separate resource directory for tests. This is essentially what MATSim does as well.

… to split legs for parking

colinsheppard

Reviewable status: 115 of 121 files reviewed, 15 unresolved discussions (waiting on @sfwatergit, @colinsheppard, and @fdariasm)

src/main/scala/beam/agentsim/Resource.scala, line 61 at r1 (raw file):