add at_reg_id_event

[aleeciu]

This PR addresses #595

[chahank]

Why make it a pandas series instead of a numpy array?

[aleeciu]

for two reasons:

• I find it cleaner as you know what impact belongs to what region simply by looking at the dataframe
• I like the pd.Series.unique() method because it retains the order. Doing the same with numpy is more cumbersome, to my knowledge.

[chahank]

I would argue that this is a misuse of a pandas series.

I find it cleaner as you know what impact belongs to what region simply by looking at the dataframe
That is independent on using numpy arrays or pandas series. But using a pandas series when it is not needed is imho actually less clean.
I like the pd.Series.unique() method because it retains the order. Doing the same with numpy is more cumbersome, to my knowledge.
I would argue that this is suboptimal as an important point is now hidden in the subtle internal working of the pandas.series.unique() vs the np.unique() methods.

@peanutfun : what do you think?

[aleeciu]

• on the first point you are right: having a pd.Series is not strictly needed to generate a pd.DataFrame

• on the second: I am transforming agg_regions into pd.Series only when the user supplies it as an np.array or list. But the user can supply a pd.Series in the first place. So I don't think it's a misuse, I just wrote the code in a way that it works with a pd.Series.

So I think your doubt is rather: should we use pandas for things that can be done with numpy? Not sure I have a clear answer to that, as this would probably apply to any code that make use of pandas.

[chahank]

The point is that pd.series are objects that are made to be used in pandas dataframes, but they are not made to be used a standalone objects. They are essentially numpy arrays, with some more. Thus, instead of making an unclear use of the pd.series, I suggest to use numpy arrays, and to make the ordering requirement clear (as currently this is a hidden feature).

[peanutfun]

I don't really see how the order is important because the order in the agg_regions parameter is determined by the "order" of the exposure points. Also, the order of columns in the final data frame does not appear relevant to me 😅

I would actually argue that the unordered result of pandas.Series.unique does not need further explanation, whereas I feel we should add a note to the docstring that the unique items will be ordered when using np.unique. So, I think the easiest solution is to just stick to the current implementation.

@chahank: Following our discussion on the "clean" indexing, I just realized that pandas.Series.unique returns a numpy array, so everything is fine from that perspective.

[aleeciu]

You are right, the order does not count. I am now using only numpy arrays.

[emanuel-schmid]

I would argue that this is a misuse of a pandas series.

Not necessarily. According to the docs a Series is a "one-dimensional ndarray with axis labels (including time series)". Sounds useful also outside of a DataFrame.
🤷

[peanutfun]

I very much agree with @emanuel-schmid here 😃

I'm fine with the current implementation. However, please make sure to only call np.unique once, and store the result for later use. It is currently executed twice and will be costly for large arrays. I can also take care of that when merging. Are we ready to go ahead?

[aleeciu]

I guess we are ready to merge

[emanuel-schmid]

Yes, looks good. 😄 I did some cosmetics in the doc string. If your happy with them @aleeciu, let me know or just commit and merge.

[emanuel-schmid]

Oh and one more thing we need to change the CHANGELOG.md

[peanutfun]

Will do and handle the merge! 👌