JuPedSim / jpsreport

Analysis tool
https://www.jupedsim.org/jpsreport_introduction.html
Other
3 stars 9 forks source link

Method A, B, D, I | remove plot option #129

Closed chraibi closed 4 years ago

chraibi commented 5 years ago

In Gitlab by @gjaeger on Oct 9, 2018, 21:12 [origin]

idea for alternative python functions string to array in issue #87

chraibi commented 5 years ago

In Gitlab by @gjaeger on Oct 9, 2018, 21:12

changed the description

chraibi commented 5 years ago

In Gitlab by @chraibi on Nov 11, 2018, 10:37

I would update the scripts and use this function instead. The function is generalised as shown in the following file.

(considers integers too)

str_to_array.ipynb

chraibi commented 5 years ago

In Gitlab by @gjaeger on Nov 11, 2018, 10:54

You have added an error handling to the function str_to_array(p)?

def str_to_array(p):
    """
    convert jpsreport polygon into <np.array>
    --> can be converted to <Polygon.Polygon> 
    """

    if not isinstance(p, str):
        raise TypeError('str_to_Array argument must be str')

    pat = re.compile(r'''(-*\d+\.?\d*, -*\d+\.?\d*),*''')
    matches = pat.findall(p)
    lst = []
    if matches:
        lst = [tuple(map(float, m.split(","))) for m in matches]
    else:
        print("WARNING: could not convert str to list")

    return np.array(lst)

The issue is on my list.

chraibi commented 5 years ago

In Gitlab by @chraibi on Nov 11, 2018, 12:48

Yes, error handling, but most importantly the pattern pat changed.

Before it detects numbers like 1.3 and ignores numbers like 13.

The new pattern considers both cases.

chraibi commented 5 years ago

In Gitlab by @gjaeger on Nov 11, 2018, 12:54

thanks. For me to remember:

old:

pat = re.compile(r'''(-*\d+\.\d+, -*\d+\.\d+),*''')

new:

pat = re.compile(r'''(-*\d+\.?\d*, -*\d+\.?\d*),*''')
chraibi commented 5 years ago

In Gitlab by @gjaeger on Nov 19, 2018, 09:21

idea (snippet):

for f in range(dfo['Frame'].min(),dfo['Frame'].max()):

    sm = cm.ScalarMappable(cmap = cm.jet)
    sm.set_clim(vmin=0, vmax=10)

    fig = plt.figure(figsize=(12, 16))

    ax1 = fig.add_subplot(111,aspect='equal')

    ax1.set_xlim(X1,X2)
    ax1.set_xlabel('x / m')

    ax1.set_ylim(Y1,Y2)
    ax1.set_ylabel('y / m')

    # geometry

    divider = make_axes_locatable(ax1)

    cax1 = divider.append_axes("right", size="5%", pad="2%")

    for i in dfo[dfo['Frame'] == f]['PersID'].iteritems():

        # Polygon von jedem Agent - Bearbeitungsschritte
        # 1. Umwandlung von str_to_array 
        # 2. Einheit anpassen   
        Polygon_Agent = str_to_array(dfo['Voronoi_Polygon'][i[0]].strip())/10000

        # Ausgabe der Dichte fuer Darstellung mit colorbar
        density = dfo[dfo['Frame'] == f]['rho'][i[0]]

        sm.set_array(density)
        sm.autoscale_None()

        ax1.add_patch(ppolygon(Polygon_Agent, 
                               fc= sm.to_rgba(density), 
                               ec='white', 
                               lw=1))

    fig.colorbar(sm, cax=cax1, label=r'density / $P/m^2$');    

    # safe

    plt.close()
gjaeger commented 5 years ago

@chraibi Calling python scripts slows down the execution of the program. The scripts were created for certain datasets. I suggest that we close this issue. The effort to maintain the files is too much.

In the upcoming version 0.8.5 I would remove the python call. As a replacement, I would create a script that can be used after running jpsreport.

mirakuepper commented 5 years ago

I agree with @gjaeger. Especially switching for example plot_graphes ="true" slows jpsreport down a lot. The Python calls as well disturb all workflow as the Python rocket is always in the foreground. I would therefore suggest to give users the possibility to execute the e.g. plotting-scripts only afterwards and erase the direct possibility in the ini-file.

chraibi commented 5 years ago

I understand, that calling Python to plot the results is slow. Matplotlib is not designed for plotting hundreds of files.

However, plot_graphes default value is false.

I also prefer a workflow as follows:

  1. Calculate everything there is to calculate.
  2. When done, plot if the user wish it, otherwise exit.

I did not check every single call, but this screenshot tells me that the plotting-scripts are indeed executed after the calculations are done.

Screenshot 2019-09-06 08 56 35

Where do we have the case where calculations and plotting are executed alternatively?

gjaeger commented 5 years ago

To the screenshot: The plotting-scripts is executed after the calculation of a frame, not after the end of all calculations.

I would therefore suggest to give users the possibility to execute the e.g. plotting-scripts only afterwards and erase the direct possibility in the ini-file.

I agree with @mirakuepper. I would remove the plot option from the ini-file.

chraibi commented 5 years ago

Is it possible to separate both parts?

For example, Analysis.cpp can be changed to:

for(long unsigned int i=0; i < _areaForMethod_C.size(); i++)
{
               Method_C method_C;
               method_C.SetMeasurementArea(_areaForMethod_C[i]);
               bool result_C =method_C.Process(data,_areaForMethod_C[i]->_zPos);
               if(result_C)
               {
                    Log->Write("INFO:\tSuccess with Method C using measurement area id %d!\n",_areaForMethod_C[i]->_id);
                    std::cout << "INFO:\tSuccess with Method C using measurement area id "<< _areaForMethod_C[i]->_id << "\n";
               }
               else
               {
                    Log->Write("INFO:\tFailed with Method C using measurement area id %d!\n",_areaForMethod_C[i]->_id);
               }
          }
     }

The removed if(_plotTimeseriesC[i]) can be added after the loop in an own loop.

gjaeger commented 5 years ago

Is it possible to separate both parts?

Yes.

The removed if(_plotTimeseriesC[i]) can be added after the loop in an own loop.

I would completely remove the plot option. Instead I would write a manual in the script folder how to use the scripts independently from jpsreport. The scripts have to be adapted according to the case. We should not link this to jpsreport.

chraibi commented 5 years ago

Sounds good.

gjaeger commented 5 years ago

Then I'll prepare a pull request.

gjaeger commented 5 years ago

ToDo:

remove plot option in

update:

gjaeger commented 4 years ago

This issue can be closed. The changes have been adopted in various PR.