moja-global / FLINT.Module.Agricultural_Soil_Model

Code repository for FLINT agricultural soils module.
Mozilla Public License 2.0
1 stars 6 forks source link

EventQueue: events don't occur if in random order #7

Closed sulays closed 4 years ago

sulays commented 4 years ago

Describe the bug If eventqueue has events in the order e1,e2,e3 with their dates in the order d1\<d2>d3 then event e3 won't occur.

To Reproduce

"eventqueue": {
        "flintdata": {
          "library": "internal.flint",
          "type": "EventQueue",
          "settings": {
            "events": [
              {
                "date": {
                  "$date": "1921/02/01"
                },
                "id": 1,
                "type": "agri.NFertEvent",
                "name": "Synthetic fertilizer",
                "quantity": 100,
                "runtime": 1
              },
              {
                "date": {
                  "$date": "1922/05/01"
                },
                "id": 2,
                "type": "agri.NFertEvent",
                "name": "Organic fertilizer",
                "quantity": 200,
                "runtime": 1
              },
              {
                "date": {
                  "$date": "1921/08/01"
                },
                "id": 2,
                "type": "agri.NFertEvent",
                "name": "Synthetic fertilizer",
                "quantity": 200,
                "runtime": 1
               }
            ]
          }
        }
}

Here, the second synthetic fertilizer event won't run.

Reason This is because of a break statement in the calendarandeventflintdatasequencer.cpp

Solution Instead of this, if (curEvent._date < curStepDate) { ++nextEvent; continue; } if (curEvent._date >= endStepDate) break;

if we do this, then it keeps on checking for the nextEvent instead of breaking the loop and even if the events are in random order they'll be activated.

if (curEvent._date < curStepDate) { ++nextEvent; continue; } if (curEvent._date >= endStepDate) { ++nextEvent; continue; }

sulays commented 4 years ago

@leitchy, should I create a PR regarding this issue or should I always give event input in ascending order? The issue is that if there are many events then it will take a lot of time to go through all of the events at each step.

leitchy commented 4 years ago

@sulays good work. Yes it would take time. However, when the event queue is in the configuration file we are generally running a point simulation, so speed isn't the most important aspect. In creating the event queue dynamically (in a build land unit module - as we discussed in Slack) you would insert them in order. So the problem goes away. If we were to add sorting as default then it would slow down the spatial simulations.

sulays commented 4 years ago

@leitchy, in eventqueue.h we are using vector to define the queue. Instead of using vector can we use something like a priority_queue so during insertion of events it will automatically sort them according to date?

leitchy commented 4 years ago

Hi Sulay,

We could put in a request to do that in the FLINT. But my point from above remains. The event queue is normally only defined in the config in a point simulation. In a spatial simulation it will be built in a module for each land unit. In both cases it's the users responsability to guarntee the order.

The expense to add a sorted list in the base, while not huge, while have an impact on simulations with large event queues, simulating millions/billions of land units.

it's less expoensive to require the items to be in order.

I think it's better to document the requirement.

sulays commented 4 years ago

@leitchy, the problem occurs due to the runtime of emissions. For example, if synthetic fertilizer is applied and the runtime is 5 days, then 4 emission events are added at the back of the eventqueue in my implementation. This results in a random ordering of the events.

If you could suggest a different solution to implement this runtime, that would be great. Or else there is a need to sort events.

leitchy commented 4 years ago

I see the problem, our pattern is not to modify the event queue after the initial buildlandunit. But it seems to make sense to your modules.

Firstly, I don't think the iteration over the event queue in CalendarAndEventFlintDataSequencer is safe for list modificatioin?

Perhaps your best approach to move past this is to write your own module for sequencing (a new CalendarAndEventFlintDataSequencer).

Then you can add a safe method for iterating a list that gets modified during the processing.

Maybe even implement a sort as you suggested? That might require extending the underlying event queue type (flint::EventQueue).

leitchy commented 4 years ago

First thing to check is if the iteration in CalendarAndEventFlintDataSequencer is safe with inserts? As it maintains a iterator during the processing and your module is inserting to the same list. I don't think it is.

Then, to get this up and going, duplicate the CalendarAndEventFlintDataSequencer is your library and modify it to iterate the event queue differently. Rather than keeping an iterator, you can traverse the event queue each time to determine which if any event is next.

Your config would need to point to the new sequencer you define:

{ "LocalDomain": { ... "sequencer_library": "internal.flint", "sequencer": "CalendarAndEventFlintDataSequencer",

We can look later at extending the event queue (flint::EventQueue) to be sorted on insert. But this would need to include a sequence change anyway because of the method of iteration (assuming it's not safe).

leitchy commented 4 years ago

I also note that your additional events are all in PreTimingSequence so you could just sort in place.

Something like this might work:

      std::sort(
          event_queue->begin(), event_queue->end(),
          [](const flint::EventQueueItem& a, const flint::EventQueueItem& b) -> bool { return a._date < b._date; });