feat: adjust scale factor design to discussion

[kainagel]

I don't know how to quote comments in other issues, so this is copy/paste from @steffenaxer. Also, I am putting in numbers.

Dear @kainagel, the recent changes to relativeToleranceForSampleSizeFactors do not seem to be fully complete or consistent.

(1) The String getter and setter are missing, so the config is currently not written at all when you try to save your GlobalConfigGroup again.

(2) The implementation here

matsim-libs/matsim/src/main/java/org/matsim/core/config/groups/GlobalConfigGroup.java

Line 147 in e8947ad

public double getRelativeToleranceForSampleSizeFactor() {

also does not seem fully correct, because the getter will always return a value significantly greater than 0.
In the SimWrapperConfigGroup, the getter is called, but within the GlobalConfigGroup it is not called.
matsim-libs/contribs/simwrapper/src/main/java/org/matsim/simwrapper/SimWrapperConfigGroup.java

Line 102 in e8947ad

final double relativeTolerance = config.global().getRelativeToleranceForSampleSizeFactor();

(3) It would be highly desirable if an exception were not thrown immediately, but instead perhaps only a warning. After all, nothing has actually changed in the QSim logic itself in recent times. So why the hard exception now?

[kainagel]

@steffenaxer @marecabo

Re (1) (no StringGetters/Setters): I have always done it like this, leaving them out for some time until the mechanics have annealed a bit. Modifying them in code is easier than modifying them once they are in xml.

Re (2): I wrote ... + Double.MAX_VALUE, but meant ... + epsilon, i.e. Double.MIN_VALUE. Thank you very much for pointing this out, that was stupid. I also moved this over to the setter, so that now also using the field directly instead of via the setter uses the slightly increased value.

Re (3) (hard exception): Is there an actual use case where this is in the way? The recommendation to have the storage capacity factor larger than the flow capacity factor for small sample sizes does not hold any more (all papers re this issue refer to a qsim that was considerably more stochastic than the current version), and I have seen many simulations where people forget adjusting the scale factor for counts or for simwrapper and then got wrong results. (Might be the special situation of a university teacher.). Plus I would assume that people who are using matsim in between releases use it with java programming, and from there the setter is available.

Re "warning" instead of "fatal error": My impression is that warnings are widely just ignored. Maybe again special situation of a university teacher.

If this is indeed a problem, i.e. we have users using matsim from in between releases, and they are not using it from java but from xml only: Yes, we could add the StringSetters/Getters. But then we would need to be somewhat sure that this is the correct design, so we do not have to change the xml interface again later.

Alternatively, we could (for the time being) make the default value relatively large, e.g., "3", so that the most typical case of flowCapFact=0.01 and storCapFact=0.03 is included.

Any opinions on that? Thank you so much.

[Janekdererste]

I think most confusion came from the Double.MAX_VALUE mixup. Otherwise, this new constraint is rather easily retrofitted by just setting a larger tolerance value.

[steffenaxer]

I wanted to create new configs with Double.MAX_VALUE, but the value was not written at all because String setter/getter logic was not fully implemented. So I came arround with a interim in code fix. Would be best, if one has a tests that de/serialization of the config goup is working.

[steffenaxer]

@kainagel

Re (1) (no StringGetters/Setters): I have always done it like this, leaving them out for some time until the mechanics have annealed a bit. Modifying them in code is easier than modifying them once they are in xml.

That is ok, but the problem is. That the config validation is still active and started to evaluate the ratio. If one than tries to change the config.xml, the error remains, because it is not reading the value from the config.xml at all. So the only workarround was to change the code, which required a new software release.

[kainagel]

That is ok, but the problem is. That the config validation is still active and started to evaluate the ratio. If one than tries to change the config.xml, the error remains, because it is not reading the value from the config.xml at all. So the only workarround was to change the code, which required a new software release.

@steffenaxer : Does this mean that you are pulling PR versions and/or weekly releases of the matsim core, convert this to jars, and run this with xml input without any code of your own in between?

Just asking about your workflow. Or maybe I am misunderstanding.

[kainagel]

I just checked QSimConfigGroup. There are more setters/getters which are not accessible through xml:

• inflowCapacitySetting
• nodeTransitionLogic
• pcuThresholdForCapacityEasing
• usingTravelTimeCheckInTeleportation

Clearly, we can make a decision in the dev mtg to not do this any more. However, as stated I find this useful for switches about which we are not sure. And for the time being it is an accepted dialect in MATSim.

I could try to put the setting into the config dump, but generate a runtime exception if trying to set this from xml. Not sure if this is a good idea.