eqasim-org / eqasim-java

The eqasim framework features an integrated transport simulation environment. It is based on the agent-based transport simulation framework MATSim with added components for simulation of discrete choice models.
http://www.eqasim.org
GNU General Public License v2.0
23 stars 40 forks source link

fix: optional eqasim termination #204

Closed tkchouaki closed 6 months ago

tkchouaki commented 6 months ago

I am in the process of updating the ile-de-france repo to use the latest version of Eqasim and I noticed a bug when running the RunPopulationRouting script during the generation of a synthetic population. The EqasimConfigurator always adds an EqasimTerminationModule, which then requires an EqasimTerminationConfigGroup. This can lead to an error if the EqasimTerminationConfigGroup is there ithe xml but the object was not passed to the ConfigUtils.loadConfig. This will result in an error at the cast inside the EqasimTerminationConfigGroup::getOrCreate method.

To avoid forgetting to pass the EqasimTerminationConfigGroup object, this PR adds it in the EqasimConfigurator as an optional config group, in order to maintain backward compatibility.

This still doesn't solve the problem for the synthetic population generation, the config file passed to the RunPopulationRouting script is generated by RunGenerateConfig which adds the EqasimTerminationConfigGroup by default. In order to solve this, this PR makes sure this config group is removed from the config right after loading.

sebhoerl commented 6 months ago

I see the problem with the unknown config group definition, but isn't the solution simply to add the EqasimTerminationConfigGroup to EqasimConfigurator.configGroups when building the list in the constructor?

sebhoerl commented 6 months ago

Meaning if you want to deactivate the termination criterion, you would need to remove the config group, which is not so convenient, I see. But maybe it is just easier thnn to always require its presence and add a parameter activate to the config group to enable/disable adaptive termination.

tkchouaki commented 6 months ago

I see the problem with the unknown config group definition, but isn't the solution simply to add the EqasimTerminationConfigGroup to EqasimConfigurator.configGroups when building the list in the constructor?

This is done indirectly by registering it as an optional config group in the configurator.

Meaning if you want to deactivate the termination criterion, you would need to remove the config group, which is not so convenient, I see. But maybe it is just easier thnn to always require its presence and add a parameter activate to the config group to enable/disable adaptive termination.

RunPopulationRouting is a special case because just instantiating the EqasimTerminationModule will result in a failure because it requires an OutputDirectoryHierarchy to be injected, which is not built during RunPopulationRouting. Edit: more precisely the OutputDirecotryHierarchy is required in the provideTerminationWriter static method.

So if we want to fix this with a parameter in the config group, we should make the entry point module not require anything and then instantiate a module equivalent to the current EqasimTerminationModule only if 'activate' is true.

tkchouaki commented 6 months ago

About the last two commits

tkchouaki commented 6 months ago

Then I ran into a problem with synpp where the org.eqasim.ile_de_france.RunSimulation script in the matsim.output stage was crashing due to ImageIo requiring a vendor name and a version to be mentioned in manifest of the ile_de_france jar file. This has been observed with java corretto-17.0.10. The last commit fixes this issue. Maybe it would be interesting to do something more general for all the territory modules (ile_de_france, los_angeles...) and have a standalone jar generated for each of them ?

tkchouaki commented 6 months ago

as of the latest commit, synpp works well (tested on Gironde).