optimize concatenation of centroids

[NicolasColombi]

Changes proposed in this PR:

Refer to #988 for detailed information on the profiling results.

Modify hazard.append() function in climada/hazard/centroids/centr.py so that concatenation of centroids is made only once, when all centroids has already been appended, instead of concatenating every time a centroid is appended (extremely time consuming)

• Create centroid attribute batch_gdf in hazard.append()
• Append centroids to batch_gdf
• Define concatenation function: finalize_append()
• Call finalize_append() to concatenate at the end of the appending process
• Clear batch_gdf attribute

This PR fixes #988

Optimize the function TropCyclone.from_tracks from 53 minutes to 4.03 minutes.

PR Author Checklist

• Read the Contribution Guide
• Correct target branch selected (if unsure, select develop)
• Descriptive pull request title added
• Source branch up-to-date with target branch
• Documentation updated
• Tests updated
• Tests passing
• No new linter issues
• Changelog updated

PR Reviewer Checklist

• Read the Contribution Guide
• CLIMADA Reviewer Checklist passed
• Tests passing
• No new linter issues

[peanutfun]

@NicolasColombi Thanks for coming up with this bugfix! If I understand correctly, the problem is that union iterates over multiple others, and for each of these calls append, which in turn calls the costly pd.concat each time. Indeed, concat creates copies, so you should call it as rarely as possible with as many arguments as possible, see https://pandas.pydata.org/pandas-docs/stable/user_guide/merging.html#concat.

You solve the issue by introducing a new attribute and function, which I think is error-prone. I would suggest that append should be updated to support a list of other centroids. union can then call append right away with the list.

   def append(self, *centr):
        for cc in centr:
            if not u_coord.equal_crs(self.crs, cc.crs):
                raise ValueError(
                    f"The given centroids use different CRS: {self.crs}, {cc.crs}. "
                    "The centroids are incompatible and cannot be concatenated."
                )
        self.gdf = pd.concat([self.gdf] + [cc.gdf for cc in centr])

I think this is a better solution because calling append (and hence concat) multiple times seems to be bad practice, and with the new signature we suggest doing otherwise.

[NicolasColombi]

@NicolasColombi Thanks for coming up with this bugfix! If I understand correctly, the problem is that union iterates over multiple others, and for each of these calls append, which in turn calls the costly pd.concat each time. Indeed, concat creates copies, so you should call it as rarely as possible with as many arguments as possible, see https://pandas.pydata.org/pandas-docs/stable/user_guide/merging.html#concat.

You solve the issue by introducing a new attribute and function, which I think is error-prone. I would suggest that append should be updated to support a list of other centroids. union can then call append right away with the list.

   def append(self, *centr):
        for cc in centr:
            if not u_coord.equal_crs(self.crs, cc.crs):
                raise ValueError(
                    f"The given centroids use different CRS: {self.crs}, {cc.crs}. "
                    "The centroids are incompatible and cannot be concatenated."
                )
        self.gdf = pd.concat([self.gdf] + [cc.gdf for cc in centr])

I think this is a better solution because calling append (and hence concat) multiple times seems to be bad practice, and with the new signature we suggest doing otherwise.

Hi @peanutfun, fine by me! With the way you propose we could even get rid completely of union() correct ? As if I understand you right, you would make append() do the job of union() with the constraints of concatenating the already appended list (hence, doing it only once and thus being fast). Or as you said, we make union call append with directly the list, but then, union() wouldn't do anything else except calling append(), or am I missing something ?

[NicolasColombi]

@NicolasColombi Thanks for coming up with this bugfix! If I understand correctly, the problem is that union iterates over multiple others, and for each of these calls append, which in turn calls the costly pd.concat each time. Indeed, concat creates copies, so you should call it as rarely as possible with as many arguments as possible, see https://pandas.pydata.org/pandas-docs/stable/user_guide/merging.html#concat.
You solve the issue by introducing a new attribute and function, which I think is error-prone. I would suggest that append should be updated to support a list of other centroids. union can then call append right away with the list.

   def append(self, *centr):
        for cc in centr:
            if not u_coord.equal_crs(self.crs, cc.crs):
                raise ValueError(
                    f"The given centroids use different CRS: {self.crs}, {cc.crs}. "
                    "The centroids are incompatible and cannot be concatenated."
                )
        self.gdf = pd.concat([self.gdf] + [cc.gdf for cc in centr])

I think this is a better solution because calling append (and hence concat) multiple times seems to be bad practice, and with the new signature we suggest doing otherwise.

Hi @peanutfun, fine by me! With the way you propose we could even get rid completely of union() correct ? As if I understand you right, you would make append() do the job of union() with the constraints of concatenating the already appended list (hence, doing it only once and thus being fast). Or as you said, we make union call append with directly the list, but then, union() wouldn't do anything else except calling append(), or am I missing something ?

[peanutfun]

With the way you propose we could even get rid completely of union() correct ?

Not necessarily. It's a bit unclear, but append and union do different things:

• append adds more centroids to the current object
• union creates a copy of the current object, adds more centroids, removes duplicate points, and returns the new object

Arguably, append is a bit superfluous. But I would not want to remove an established function as a reaction to a bug report.

[NicolasColombi]

Not necessarily. It's a bit unclear, but append and union do different things:

Yes, that is why I thought that a single function could do both, but as you mentioned it is probably better to keep them separate, one of the reason being that all tests pass and that it shouldn't compromise the rest of the code this way.

[peanutfun]

Thanks for updating the PR! It looks much cleaner now. Please fix the remaining small issues and update the PR description.

[peanutfun]

Please fix the linter issue. cc is too short, maybe use other?

[peanutfun]

Please add a dedicated test for append with multiple arguments

[NicolasColombi]

@peanutfun I added a test to verify that append() can accept multiple arguments, the docs looks good with :py:meth:, the changelog is also updated now as you suggested.

[bguillod]

Thanks a lot for this improvements, it makes a lot of sense and the performance gain is significant indeed! A few small comments but looks good to me overall.

[bguillod]

since centr is given as a *args, it does not have to be a list but can be several centroids, no? Formulated differently, can this method be called these two ways with equivalent results or does only one way work?

centr.append(new_centrs1, new_centrs2)

centr.append([new_centrs1, new_centrs2])

[sarah-hlsn]

To my understanding, the code as currently written would work if you call either centr.append(new_centrs1, new_centrs2) or centr.append(*[new_centrs1, new_centrs2]) but would not work calling centr.append([new_centrs1, new_centrs2]), since it wouldn't unpack the list into single centroid objects before the loop for other in centr which checks the CRS.

@NicolasColombi and @peanutfun, should we maybe consider adapting the method so that it would also work when passing a list without *?

[NicolasColombi]

Hi Benoit, I think you are right, I will update the docs strings, it shouldn't be a list.

[bguillod]

Same comments as above: if it's an *args then it's not a list, no?

[sarah-hlsn]

Looks really good to me, thanks a lot!
Just fixed some minor typos in the docstrings.

As @bguillod highlighted, there could be some inconsistency associated with how the centr argument is passed to the method/how it is defined in the docstrings. I think we should either add a fix to support different ways of passing this argument or clarify the docstrings (if my understanding is correct here, happy to defer to your expertise @NicolasColombi & @peanutfun).

[sarah-hlsn]

To my understanding, the code as currently written would work if you call either centr.append(new_centrs1, new_centrs2) or centr.append(*[new_centrs1, new_centrs2]) but would not work calling centr.append([new_centrs1, new_centrs2]), since it wouldn't unpack the list into single centroid objects before the loop for other in centr which checks the CRS.

@NicolasColombi and @peanutfun, should we maybe consider adapting the method so that it would also work when passing a list without *?

[NicolasColombi]

@sarah-hlsn exactly! I personally wouldn't modify it so that it can accept a list.

[sarah-hlsn]

@sarah-hlsn exactly! I personally wouldn't modify it so that it can accept a list.

Makes perfect sense, we can just adapt the docstrings as you say :)

[NicolasColombi]

May I merge ? 👍 @peanutfun