-
Notifications
You must be signed in to change notification settings - Fork 57
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
SkimPlus & RepositioningManager #1585
Conversation
test! |
test! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor formatting item to take or leave :)
…d demand collected by the skimmer
test! |
test! |
test! |
Is this still wanted. It went stale, so I'm not sure. If it is then I can fix the current compiler errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be pointing to master or dev? It seems like a heavy one to go straight to master
@@ -190,6 +196,16 @@ class BeamSkimmer @Inject()(val beamConfig: BeamConfig, val beamServices: BeamSe | |||
} | |||
} | |||
|
|||
def getPreviousSkimValueOrDefault(time: Int, mode: BeamMode, orig: Id[TAZ], dest: Id[TAZ]): Skim = { | |||
previousSkims.get((timeToBin(time), mode, orig, dest)) match { | |||
case someSkim @ Some(_) => someSkim.get.toSkimExternal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be rewritten like case Some(skim) => skim.toSkimExternal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
label: Label, | ||
who: Option[Id[Person]] | ||
): Unit = { | ||
who match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just need to do some side-effect on who
in case it is Some
, you could use foreach
:
who.foreach { _ =>
val timeBin = timeToBin(time, timeWindow)
val taz = beamServices.tazTreeMap.getTAZ(location.getX, location.getY)
val key = (timeBin, taz.tazId, vehicleManager, label)
skimsPlus.put(key, skimsPlus.getOrElse(key, 1.0) + 1.0)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I already removed the who from the parameters. I realized afterwards that I don't need it.
var destTimeOpt: Option[(RepositioningRequest, Int)] = None | ||
undersuppliedTAZ.foreach { dst => | ||
val skim = beamSkimmer.getPreviousSkimValueOrDefault(startTime, BeamMode.CAR, org.taz.tazId, dst.taz.tazId) | ||
if (destTimeOpt.isEmpty || (destTimeOpt.isDefined && skim.time < destTimeOpt.get._2)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be rewritten like, but not sure that it looks better :D
destTimeOpt match {
case None =>
destTimeOpt = Some((dst, skim.time))
case Some((_, time)) if skim.time < time =>
destTimeOpt = Some((dst, skim.time))
case _ =>
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case I prefer the "if" since there are less line of codes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as an FYI, I don't think you gain much in allocation in the current mutable state, so maybe a foldLeft would be better overall?
oversuppliedTAZ.foreach{ org =>
undersuppliedTAZ.foldLeft(None: Option[(RepositioningRequest, Int)]){ case (destTimeOpt, dst) =>
val skim = beamSkimmer.getPreviousSkimValueOrDefault(startTime, BeamMode.CAR, org.taz.tazId, dst.taz.tazId)
if(destTimeOpt.isEmpty) Some(dst, skim.time)
else destTimeOpt.collect{ case destTime if skim.time > destTime._2 => Some((dst,skim.time))}.getOrElse(destTimeOpt)
}.foreach{ case (dst, time) => ODs.append((org, dst, time))}
}
destTimeOpt = Some((dst, skim.time)) | ||
} | ||
} | ||
destTimeOpt match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If just side-effect should happen, also could use destTimeOpt.foreach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
log.debug("Checked out " + token.id) | ||
case _ => | ||
who ! NotAvailable | ||
println(s"to board ===> ${token.id}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is preferable to use logger.debug
and adjust debug level for particular class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already removed all these println lines
RepositionManagerListener.writeRepositionEvents(event, eventsList) | ||
} | ||
|
||
val eventsList = mutable.ListBuffer.empty[(Int, Id[TAZ], Id[VehicleManager], Id[BeamVehicle], String, String)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many events will be here? Could it cause an issue (memory) on production run with a lot of vehicles? If it is stored only for the purpose of writing it in the end of iteration, it can be written during the simulation instead of accumulating all events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is the big change I did since yesterday. I got rid of the whole listener thing and rather used the skimmer with more or less same purpose
test/input/beamville/beam.conf
Outdated
managerType = "fixed_non_reserving_fleet_from_file" | ||
fixed_non_reserving_fleet_from_file { | ||
vehicleTypeId = "PHEV", | ||
filename = "/Users/haitam/workspace/beam/test/input/beamville/vehiclesShared.csv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, make sure this path exist everywhere (maybe need to add it under sourcecontrol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad didn't plan to push the conf file
test/input/beamville/beam.conf
Outdated
beam.warmStart.enabled = false | ||
beam.warmStart.path = "https://s3.us-east-2.amazonaws.com/beam-outputs/run149-base__2018-06-27_20-28-26_2a2e2bd3.zip" | ||
beam.warmStart.enabled = true | ||
beam.warmStart.path = "/Users/haitam/workspace/beam/output/beamville/beamville__2019-05-13_17-18-48/ITERS/it.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, make sure this path exist everywhere (maybe need to add it under sourcecontrol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
mutable.TreeSet.empty[RepositioningRequest](Ordering.by[RepositioningRequest, Int](_.shortage)) | ||
|
||
beamServices.tazTreeMap.getTAZs.foreach { taz => | ||
if (relocate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing happens in the loop if relocate
is false, so it can be taken out of loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay I changed this code too
test! |
new skim data structure that should include additional information associated to every TAZ, such available vehicles by VehicleManager
Closes #1540
This change is