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

Sync up with master: Part 1 #1712

Merged
merged 15 commits into from
Apr 24, 2019
Merged

Sync up with master: Part 1 #1712

merged 15 commits into from
Apr 24, 2019

Conversation

REASY
Copy link
Collaborator

@REASY REASY commented Apr 18, 2019

This change is Reviewable

@ghost ghost assigned REASY Apr 18, 2019
@ghost ghost added the Review label Apr 18, 2019
@REASY
Copy link
Collaborator Author

REASY commented Apr 18, 2019

test!

@REASY
Copy link
Collaborator Author

REASY commented Apr 18, 2019

@carloscaldas, @JustinPihony
What do you think should be proper way to bring those changes back from master to develop? I've simply cherry-picked those commits from master which are not in develop (my changes) via git cherry origin/develop origin/master

Copy link
Contributor

@colinsheppard colinsheppard left a comment

Choose a reason for hiding this comment

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

LGTM

@REASY
Copy link
Collaborator Author

REASY commented Apr 19, 2019

test!

@JustinPihony
Copy link
Collaborator

@REASY I think cherry pick is a fine way to go

@carloscaldas
Copy link
Collaborator

@REASY one thing that I dislike in gitflow is that we cannot protect master branch (due to hotfixes) and cannot make sure a test ran before merging. Since the need happened cherry-pick is ok. But it is better to avoid merging in master unless is a hotfix.

val w = new Way()
w.addTag(WayFixer.LANES_TAG, "3");
WayFixer.fixLanes(1, w) shouldBe false
w.getTag(WayFixer.LANES_TAG) should be("3")
Copy link
Collaborator

Choose a reason for hiding this comment

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

extract a method for those duplicate blocks of code

// private[osm] def getFixedSpeed(osmId: Long, way: Way): Option[Double] = {
// Option(way.getTag(MAXSPEED_TAG)) match {
// case Some(rawMaxSpeed) =>
// if (rawMaxSpeed.startsWith("[")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code

EdgeStore.Edge cursor = r5Network.streetLayer.edgeStore.getCursor(); // Iterator of edges in R5 network
OsmToMATSim OTM = new OsmToMATSim(mNetwork, true);

int numberOfFixes = 0;
HashMap<String, Integer> highwayTypeToCounts = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Map<String, Integer> highwayTypeToCounts = new HashMap<>();

Copy link
Collaborator

@carloscaldas carloscaldas left a comment

Choose a reason for hiding this comment

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

Added comments and approve to not block

@REASY
Copy link
Collaborator Author

REASY commented Apr 19, 2019

test!

@ghost ghost assigned JustinPihony Apr 20, 2019
@JustinPihony
Copy link
Collaborator

test!

3 similar comments
@REASY
Copy link
Collaborator Author

REASY commented Apr 20, 2019

test!

@REASY
Copy link
Collaborator Author

REASY commented Apr 21, 2019

test!

@REASY
Copy link
Collaborator Author

REASY commented Apr 21, 2019

test!

@JustinPihony
Copy link
Collaborator

The current build is failing because it is failing in master already...per b440293 Many tests are not set up for this injection - https://github.com/LBNL-UCB-STI/beam/blob/master/src/main/scala/beam/agentsim/agents/PersonAgent.scala#L220

@JustinPihony
Copy link
Collaborator

I'll put together a fuller PR when I get back to this (have to stop for now) - but here is a quick fix I did that works for the OtherPersonAgentSpec:

trait InjectableMock extends MockitoSugar {
  val injector  = mock[com.google.inject.Injector](withSettings().stubOnly())
  val config: BeamConfig
  val beamSvc: BeamServices
  private val x = new beam.router.TravelTimeObserved(config, beamSvc)
  when(injector.getInstance(classOf[beam.router.TravelTimeObserved])).thenReturn(x)
}

...

with InjectableMock

...

when(theServices.injector).thenReturn(injector)

Note that this is a bit of a circular reference, but solved by laziness...

@REASY
Copy link
Collaborator Author

REASY commented Apr 23, 2019

test!

@REASY
Copy link
Collaborator Author

REASY commented Apr 23, 2019

test!

2 similar comments
@REASY
Copy link
Collaborator Author

REASY commented Apr 23, 2019

test!

@REASY
Copy link
Collaborator Author

REASY commented Apr 24, 2019

test!

@REASY
Copy link
Collaborator Author

REASY commented Apr 24, 2019

test!

1 similar comment
@REASY
Copy link
Collaborator Author

REASY commented Apr 24, 2019

test!

@JustinPihony
Copy link
Collaborator

test!

@JustinPihony JustinPihony merged commit 42d6e56 into develop Apr 24, 2019
@ghost ghost removed the Review label Apr 24, 2019
@JustinPihony JustinPihony deleted the art/sync-up-with-master branch April 24, 2019 17:11
REASY added a commit that referenced this pull request May 16, 2019
* od skimm scatter plot

* add legend to scatter plot

* put values in buckets for color clustering

* fix formatting for TravelTimeObserved

* change how buckets work for TravelTimeObserved

* sort buckets for TravelTimeObserved

* fix formatting

* fix shape of the dots

* fix formatting of TravelTimeObserved

* - Added hashCode (#1675)

- Changed the order to make sure light operation happens at first

* Use `R5` from `com.github.LBNL-UCB-STI`

* Events: Added linkTravelTime to attributes of PathTraversalEvent (#1671)

* Added `linkTravelTime` to attributes of `PathTraversalEvent`

* Make sure it is backward compatible

* fix: Motorized vehicle miles traveled should not includes walk (#1704)

* Add slack notification for beam deployments (#1718)

* Removed redundant map

* Removed unused imports

* Simplified expression and removed warning

* Removed warning: unnecessary parenthesis

* Removed warning: replaced map.get(key).get with map(key)

* Removed warning: added type annotation to unit method

* Removed warning: simplified expression

* Removed two warnings: replace.size>0 with nonEmpty

* Removed warnings: named parameters

* remove redundant jar

* scala fmt

* set write matsim network to true by default

* add logs to beam skimmer

* new histogram for all roads

* log activities with non-negative end times

* Sync up with master: Part 1 (#1712)

* - Fix links with the same `fromNode` and `toNode`: set length, capacity and freespeed
- Added `WayFixer` to fix OSM data (highway type and lanes)

* Updated `beam-utilities` to `v0.2.1`

* - `injector` is a field of `BeamServices` (it's already god object, won't make it worse :D)
- `TravelTimeObserved` has it's own skimmer
- TravelTimeObserved should use only `CAR` and `CAV`

* Log number of times when min speed has been used during the iteration

* Fix TravelTimeObserved related tests

* fmt

* Fix compilation for publicity and naming

* Add all modes to excerpt skims

* Fix trait due to circular pain

* fmt

* Prepare data in parallel

* Provide `BeamSkimmer` and `TravelTimeObserved`

* Done

* Move to handle timing

* Fixed broken tests

* Fixed `Can' t connect to X11 window server using 'localhost:10.0' as the value of the DISPLAY variable`

* Changed HouseHoldInfo.cars from Double to Int (#1747)

* Use 2-d array to represent travel time (#1716)

* - File appender is described in `logback.xml`. Path is injected via property (#1657)

- Simplified `logback.xml`

* Write csv and plot to compare euclidean vs. length attribute plot

Write csv and plot to compare euclidean vs. length attribute plot for
matsim network #1697

* R5RoutingApp to be able to make routing request via HTTP Post request (#1670)

* R5RoutingApp to be able to make routing request via HTTP Post request

* Make sure warm-start is taken into account

* Extract formats to `beam.utils.json` package to be able to reuse them

* fix exclusion syntax for all iterations

* histogram to check link length distributions within the network

* - Safe previously created attributes (#1755)

- Memory optimization for XML event writer
- Do not allocate extra HashSet, map already contains keys as Set `keySet`

* add linkId column to writeComparisonEuclideanVsLengthAttributeCsv

* Removed startup message duplication

* Removed warning enclosing block is redundant

* Removed warning - shadowing pattern

* Minor improvements for `getClosestIdleVehiclesWithinRadiusByETA` - do not sort, just use `min` (#1758)

* publish changes made to beam config during run time ,to all references using observable design pattern

* NetworkXmlToCSV.scala: Ability to run (#1775)

* - Added `main` method
- Ability to read compressed XML (.xml.gz)

* Make sure comma is escaped

* convert physsim event xml to csv

* reset configfilelocations system property on shutdown

* add beam scoring function factory and hao2018 as observers

* Escape comma for the mergeOutput too

* Avoid scenario loaded twice 4ci (#1766)

* Added test for BeamSkimmer CSV read/write and small refactoring (#1784)

* fixed conversion from double as string to int (#1785)

* add AgentSimtoPhysSimPlanConvertor as beam config change observer

* add BeamScoringFunctionFactory as beam config change observer

* listen to beam config changes in notify iteration ends inside beam sim

* format code

* Project: Split logback to dev (original one) and prod (#1763)

* - Added `src/main/resources` to resources
- Added `logback_prod.xml` for PROD
- Set `logback.xml` in `RunBeam` if it is not set
- Added `log-path_IS_UNDEFINED` to gitignore

* Added new line

* Fixed PR review

* inject current beam config instance whenever the config path is not found

* Fix Warm start routing Spec

* remove unwanted Test class used for debugging
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.

5 participants