-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Calculate CO₂ emissions of itineraries #5278
Conversation
…ule data fetching since the utility method was removed. Also initial unit tests added
docs/sandbox/DigitransitEmissions.md
Outdated
|
||
Emissions data should be in json array with objects that have the following properties: | ||
|
||
`db`: database it refers to or CAR when referring to personal travel by car. For Example Linkki, HSL |
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.
Maybe rename this to feed_id
. Although, does it necessarily match to the feed id we use for these data sets in OTP? I think we could omit this, agency_id
and agency_name
fields for CAR travel.
docs/sandbox/DigitransitEmissions.md
Outdated
Emissions data is loaded from the provided source and embedded in graph building process | ||
as part of the itinerary. |
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.
Emissions data is loaded from the provided source and embedded in graph building process | |
as part of the itinerary. | |
Emissions data is loaded from the provided source and embedded into the graph during the build process. |
docs/sandbox/DigitransitEmissions.md
Outdated
|
||
### OTP 2.4 | ||
|
||
- Initial implementation of Emissions calculation |
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.
- Initial implementation of Emissions calculation | |
- Initial implementation of the emissions calculation |
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.
Maybe we should have a package structure like package org.opentripplanner.ext.emissions.digitransit; and then these classes shouldn't have digitransit in the name. This would allow people to add other emission implementations inside the emissions sandbox. Other option would be to name this package as digitransitemissions.
import org.opentripplanner.framework.lang.Sandbox; | ||
|
||
@Sandbox | ||
public class DigitransitEmissions implements Serializable { |
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 don't think we should load the json data into objects as such but parse it into a more sensible structure (and with more java friendly naming conventions).
public List<Itinerary> filter(List<Itinerary> itineraries) { | ||
return itineraries | ||
.stream() | ||
.peek(i -> { |
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.
Maybe forEach
instead, I'm not too familiar with this peek
method.
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.
A old fasion loop and a return statment would make it mush easier to read - also, do not create a new list - just return the list passed into this filter. It is modifying the itineraries not to list.
public interface EmissionsService extends Serializable { | ||
HashMap<String, DigitransitEmissionsAgency> getEmissionByAgency(); | ||
|
||
Float getEmissionsForRoute(Itinerary itinerary); |
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 this should be called getEmissionsForItinerary
return null; | ||
} | ||
|
||
public Float getEmissionsForRoute(Itinerary itinerary) { |
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 should be the only public method in this class.
@Sandbox | ||
public class DigitransitEmissionsService implements EmissionsService { | ||
|
||
private HashMap<String, DigitransitEmissionsAgency> emissionByAgency; |
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.
The information we need to decide what the emissions are for a leg are agency id and transport mode. I think the key therefore could be a string of something like <feedscoped agency id>_<transport mode>
and the value should be a float/double/int whatever we decide is appropriate.
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.
Then, for car, the key could just be CAR
because the agency is not relevant. Although, it could also make sense to have a special field here to store the car emission as we don't at least yet need to support emissions for other street modes.
|
||
private HashMap<String, DigitransitEmissionsAgency> emissionByAgency; | ||
|
||
public DigitransitEmissionsService(DigitransitEmissions[] emissions) { |
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 just take in the Map as the constructor parameter.
/** | ||
* This class is responsible for mapping OSM configuration into OSM parameters. | ||
*/ |
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.
Left over from copy paste, update.
public EmissionsService getEmissionsService() { | ||
return emissionsService; | ||
} | ||
|
||
public void setEmissionsService(EmissionsService emissionsService) { | ||
this.emissionsService = emissionsService; | ||
} | ||
|
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.
Sorry, we do not pass "stuff" around in the graph any more, you need to inject it using Dagger.
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.
Thanks for putting time into reviewing this draft! There are alot of changes coming soon, but you have pointed out some things I hadn't considered. I shall fix these as well before updating the pr.
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.
Unfortunately, I'm having slight issues with the injection, as I am not sure where exactly it should be done. As far as I understand, we need to add the service/filter to the builder in RouteRequestToFilterChainMapper or ItineraryListChainBuilder, but we cannot inject anything in those. Could you maybe clarify how exactly a filter should be added without the graph? Thanks!
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.
Take a look at the OtpServerRequestContext
, witch is available where the ItineraryListFilterChainBuilder
is created. You should let Dagger create your service. Then inject it in your graph-builder-module, the SerializedGraphObject
and the OtpServerRequestContext
. You need to inject it in the SerializedGraphObject
for the data to be serialized.
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'm afraid you might have to spell this out for me, as I'm failing to see how to inject the service into the SerializedGraphObject without doing something strange. The SerializedGraphObject is initialised in the OTPMain only and I doubt you meant the emissionsservice is supposed to added to the SerializedGraphObject in the OPTMain. Also, I managed to inject and get the service from the OtpServerRequestContext, but of course it is a new instance and not the one handled during the building phase that contains the emissions data. So, I'm also failing to see how the service instance (that should be) injected into the SerializedGraphObject, will end up being the same instance that is gained from the context in the RouteRequestFilterChainMapper (as opposed to the brand new singleton without emissions data that I get now). Does there happen to be any existing service that has been implemented in the manner you are suggesting that I could maybe look at? I was only able to find the ones made in the old way and one that doesn't require the build phase at all (rideHailingService). Thanks!
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 don't think there is a full example for a service, so you're breaking new ground, but the worldEnveloperRepository and dataImportIssueSummary combine data from various graph building phases and put that into the SerializedGraphObject
.
OpenTripPlanner/src/main/java/org/opentripplanner/standalone/OTPMain.java
Lines 148 to 155 in bf6e4be
new SerializedGraphObject( | |
app.graph(), | |
app.transitModel(), | |
app.worldEnvelopeRepository(), | |
config.buildConfig(), | |
config.routerConfig(), | |
DataImportIssueSummary.combine(graphBuilder.issueSummary(), app.dataImportIssueSummary()) | |
) |
@Sandbox | ||
private EmissionsService emissionsService; |
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 would prefer that you set the ItineraryListFilter emissionsFilter
here, not the service, so that the ItineraryListFilterChainBuilder
has does not need to depend on the org.opentripplanner.ext.emissions
package. I know we have a few other dependencies to the sandbox in here - because they relay on the config, but the emission seems to not have any config.
|
||
private String name; | ||
private float avg; | ||
private int p_avg; |
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.
Underscores(C style) are not allowed in any package, class, field or variable names according to our coding guideline.
You need to rebase on or merge the dev-2.x branch in order for the compilation to succeed. |
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.
The issue store entries shouldn't have too generic labels as then it's hard to figure what the errors are related to when browsing through error types.
try (InputStream feedInfoStream = new FileInputStream(directory + "/feed_info.txt")) { | ||
feedId = readFeedId(feedInfoStream); | ||
} catch (IOException e) { | ||
issueStore.add("InvalidData", "Reading feed_info.txt failed."); |
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.
issueStore.add("InvalidData", "Reading feed_info.txt failed."); | |
issueStore.add("InvalidEmissionData", "Reading feed_info.txt failed."); |
try (InputStream stream = new FileInputStream(directory + "/emissions.txt")) { | ||
emissionsData = readEmissions(stream, feedId); | ||
} catch (IOException e) { | ||
issueStore.add("InvalidData", "Reading emissions.txt failed."); |
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.
issueStore.add("InvalidData", "Reading emissions.txt failed."); | |
issueStore.add("InvalidEmissionData", "Reading emissions.txt failed."); |
zipFile.close(); | ||
return emissionsData; | ||
} catch (IOException e) { | ||
issueStore.add("InvalidData", "Reading emissions data failed."); |
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.
issueStore.add("InvalidData", "Reading emissions data failed."); | |
issueStore.add("InvalidEmissionData", "Reading emissions data failed."); |
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 in these scenarios where the whole thing fails, we can also add a log line.
|
||
if (avgCo2PerVehiclePerKmString.isEmpty()) { | ||
issueStore.add( | ||
"InvalidData", |
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.
"InvalidData", | |
"InvalidEmissionData", |
} | ||
if (avgPassengerCountString.isEmpty()) { | ||
issueStore.add( | ||
"InvalidData", |
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.
"InvalidData", | |
"InvalidEmissionData", |
reader.readRecord(); | ||
return reader.get("feed_id"); | ||
} catch (IOException e) { | ||
issueStore.add("InvalidData", "Reading emissions data failed."); |
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.
issueStore.add("InvalidData", "Reading emissions data failed."); | |
issueStore.add("InvalidEmissionData", "Reading emissions data failed."); |
} | ||
if (avgPassengerCount <= 0 || avgCo2PerVehiclePerMeter < 0) { | ||
issueStore.add( | ||
"InvalidData", |
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.
"InvalidData", | |
"InvalidEmissionData", |
There must be an error in |
The issue with grams missing? I think I just forgot to run the generation. |
No, I think the import for Grams is wrong. |
You're right, the package was wrong for some reason. |
There is a conflict here again. While you solve that, you can also address #5278 (comment) but it's not really required. |
3ab8b67
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.
There are a few things is left to fix.
- We only want the root level doc element for a feature to be present in out main doc, there rest should be in the Sandbox doc. There are several examples to follow here.
- If a Sandbox feature is not enabled then GraphBuilder module, services, models and repositories should not be created.
src/main/java/org/opentripplanner/graph_builder/GraphBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opentripplanner/standalone/api/OtpServerRequestContext.java
Show resolved
Hide resolved
Co-authored-by: Thomas Gran <[email protected]>
aa5f235
Summary
Issue
closes #4940
Unit tests
Tests added in a new file EmissionsServiceTest.java
Tests for:
Documentation
Modified:
Added:
Changelog
From title