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

Person money event extension #811

Merged
merged 13 commits into from
Feb 10, 2020
Merged

Person money event extension #811

merged 13 commits into from
Feb 10, 2020

Conversation

vsp-gleich
Copy link
Contributor

add information for what purpose a person received / paid money and from / to whom -> differentiate different kinds of payments, e.g. analyse toll revenue and drt fare revenue per operator etc.

@vsp-gleich
Copy link
Contributor Author

vsp-gleich commented Feb 6, 2020

What is the best default value for the new fields?

  • "null" (a String, not the null pointer!): This is what comes out if I read in an EventsFile with old personMoneyEvents without the new fields (null in the code which is written out as String "null"). Disadavantage: hard to tell whether it is null due to a (null pointer) bug in the code or because it should be null
  • "" the empty String: shows more that someone wanted to return this (differentiate from a bug).
  • "unknown" (as a constant like TransportMode.xxx, so no typos)
  • Let's not use the null pointer, because after writing out and reading in again it would change to a String "null" :-(

Any opinions on this?

@kainagel
Copy link
Contributor

kainagel commented Feb 7, 2020 via email

@vsp-gleich
Copy link
Contributor Author

vsp-gleich commented Feb 7, 2020

I think that for events the convention is to just not write out at all

I've just changed that and replaced the empty Strings with null as default value for empty Strings.

@vsp-gleich
Copy link
Contributor Author

There is an issue with contrib protobuf. It apparently auto-generates ProtobufEvents based on the old PersonMoneyEvent without the new fields. Does anyone know more about how protobuf works or has recently changed an event?

@michalmac
Copy link
Member

There is an issue with contrib protobuf. It apparently auto-generates ProtobufEvents based on the old PersonMoneyEvent without the new fields. Does anyone know more about how protobuf works or has recently changed an event?

Updated the proto definition. Should be fixed now.

@michalmac
Copy link
Member

michalmac commented Feb 8, 2020

Re default values:
In proto, the default value for strings is "", and you cannot set it to null (NullPointerException).

If we want to have null as the default value in the matsim events, some extra handling is necessary that bi-di maps: ""<=> null while doing conversions.

@vsp-gleich
Copy link
Contributor Author

Updated the proto definition. Should be fixed now.

Thanks a lot @michalmac 👍 !

@vsp-gleich
Copy link
Contributor Author

Re default values:
In proto, the default value for strings is "", and you cannot set it to null (NullPointerException).

If we want to have null as the default value in the matsim events, some extra handling is necessary that bi-di maps: ""<=> null while doing conversions.

Thanks for the hint, I was not aware of this. I have just looked through some other event types. PersonStuckEvent also has an optional String field (legMode). It is written analogous to the new PersonMoneyEvent fields both in matsim core and in contrib protobuf. So they should face the same difficulties.

PersonMoneyEvent.java#L90-L102

PersonStuckEvent.java#L66-L77

contribs/protobuf/src/main/proto/events.proto#L119-L132

Maybe we stick for this pull request to the current MATSim default (null, null is not written out) and move the more general discussion about default values to JIRA 1007.

@vsp-gleich vsp-gleich merged commit dcb5137 into master Feb 10, 2020
@vsp-gleich vsp-gleich deleted the personMoneyEventExtension branch February 10, 2020 12:56
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.

3 participants