gama-platform / gama.old

Main repository for developing the 1.x versions of GAMA
GNU General Public License v3.0
304 stars 99 forks source link

Charts use a wrong value for the cycle variable #3561

Closed lesquoyb closed 1 year ago

lesquoyb commented 1 year ago

Describe the bug When using the variable cycle in a chart, the value returned is the value of the cycle before the step is done instead of after it's done

To Reproduce run four steps of this model:

model chartsrefresh

global {
}

experiment a {
    output {
        display test refresh:every(3 #cycle){
            chart "chart"{
                data "cycles" value:cycle;
            }
        }
    }
}

here is the result: image

You will notice two problems:

To me it feels like it's related to previous problems we recently solved with cycles where the counter for cycles was read before the step was over.

Expected behavior While I'm not sure if we need to use the value of the cycle during the step or its value once the step is over (both having different meanings) for the data, for the refresh facet it seems really weird to have that "offset" on the first step

Desktop (please complete the following information):

Additional context No difference in behavior between 2d and 3d displays

AlexisDrogoul commented 1 year ago

I'm not completely sure how to solve this ...

There is one method in ChartOutput that goes like:

    /**
     * Gets the chart cycle.
     *
     * @param scope
     *            the scope
     * @return the chart cycle
     */
    public int getChartCycle(final IScope scope) {
        if (ismyfirststep) {
            ismyfirststep = false;
            return 0;
        }
        return scope.getClock().getCycle() + 1;
    }

But it has been there since e2c9f514fa193ef567fdbad43d620890d2db7bf8 ... 5 years ago. Changing it to return the cycle number does not change anything anyway.

There seems to be a shift (+1) between the reading of the cycle to compute the value and the displaying of the graph (y axis) ... investigating.

AlexisDrogoul commented 1 year ago

I have pushed a possible fix. However, it might impact other charts. The solution chosen is to not force the display of arbitrary values (in the series) when the chart is displayed; so charts begin empty (instead of having a value). Can you test this ? @lesquoyb

lesquoyb commented 1 year ago

It does fix the problem with the first data-point repetition, though now it seems that those displays are lagging one step behind. For example if you print every cycle the current cycle, you will see that after 4 experiment cycles you only have 3 values in the chart (stopping at value 3).

I don't think creating charts with an initial value was a bad idea, as it corresponds to the initial conditions of the experiment. I already had a feeling that we may have to revise a bit the way cycles work in gama when working on recent fixes on memorize experiments, as sometimes you need to keep count of the total elapsed cycles, experiment elapsed cycles, current cycle, previous cycle etc.

So if there's no obvious solution to this I recommend that we revert back to before the commit (as apparently it didn't bother people much during 5 years) and leave it for gama 2.0, what do you think ?

AlexisDrogoul commented 1 year ago

Actually, what you describe is completely correct.

With a model like:

model chartsrefresh

experiment a {
    output {
        display test type: 2d{
            chart "chart"{
                data "cycles" value:cycle;
            }
        }
    }
}

you obtain four data points corresponding to the four cycles (first at 0;0, last at 3;3) while the last value of cycle is 3.

So I suggest that we keep the change (I've reverted it for the moment, but it is easy to revert it back). @gama-platform/committers what is your opinion ?

hqnghi88 commented 1 year ago

Well i did not test with all chart (especially the ones for ode) but @tnguyenh did not complain anything in 5 years. Seems ok for me.

AlexisDrogoul commented 1 year ago

OK. So let's not touch anything then ! I reverted again the commit.