Add magic functions

[Teschl]

In this version, I implemented the previously discussed "magic" functions. Turns out the r and i versions (as noted in #17 ) are not needed, but instead of __cmp__ all the functions for comparing had to be implemented on their own.

I am using a copy.deepcopy() when returning the new dem (after computing and, for example) since only doing a shallow copy resulted in overwriting dem1 when doing this, for example: dem3 = dem1 & dem2

• for __len__ we need to decide, if we want to return the number of elements in the first dimension (like it works for the np.array). or i we want to return the shape of the dem (rows, cols).
• for __delitem__ I am questioning if we need it at all. I can't think of a situation, where the user would want to delete a specific row of their dem.

I included the notebook examples/example_magic_funcs.ipynb which provides simple examples that showcase functionality. Feel free to play around with those a bit.

[Teschl]

I should add, right now dem1 == dem2 returns a GridObjectwith element wise results instead of returning just Trueor False. This is probably very unexpected, so it might be smarter to implement a eq(dem1, dem2) function that returns another dem

[wschwanghart]

This is an element-wise operation in Matlab's TT, too. And I think this is fine. One could just implement an isequal-function to compare objects. This would require checking the equality of the array of elevations, coordinates, etc.

[Teschl]

I played around with subclassing a bit, it generally seems to work for our use case. I will create a new branch in my fork to try to implement it. Then we can decide if we want to go this route and merge it.

We will have to see if we run into any issues when overwriting functions such as __add__. Also, inheriting functions like .astype() from np.ndarray may be dangerous (could lead users to changing data types and then running into issues). I'll check if we can limit how much is inherited from ndarray.

[Teschl]

I think I would be against subclassing ndarray.

Since

def __array__(self):
        return self.z

ensures that you don't need to use dem.z when passing an instance to Matplotlib, for example (just dem works). There is no need to subclass for plotting.

In my opinion the issue with subclassing is, that we also inherit functions like: astype, sort, byteswap, dump or tofile. It's not like these functions should not exist for the GridObject, I think giving the user access to most of them will do more harm than good. If the user really wants to use them, they can still do dem.z.some_function.

I think we should go through the ndarray functions and choose which we want to keep. Writing a wrapper for each will probably be no big task.

Otherwise, I fear we would add a lot of functions that see no use. When overwriting a function in a subclass, we could remove the functionality, but not the existence of the function. So for example in an IDE the autocomplete would have many "empty" functions, which could prove annoying.

If you think we should still go the subclass route, I'm open to it. But as of right now, I don't see the immediate advantage.

[wschwanghart]

That’s how I do in MATLAB. And it allows you to set extra options.

Von Gmail Mobile gesendet

Theo @.***> schrieb am Mo. 13. Mai 2024 um 21:39:

I think I would be against subclassing ndarray.

Since

def array(self): return self.z

ensures that you don't need to use dem.z when passing an instance to Matplotlib, for example (just dem works). There is no need to subclass for plotting.

In my opinion the issue with subclassing is, that we also inherit functions like: astype, sort, byteswap, dump or tofile. It's not like these functions should not exist for the GridObject, I think giving the user access to them will do more harm than good. We could just overwrite them. If the user really wants to use them, they can still do dem.z.some_function.

I think we should go through the ndarray functions https://numpy.org/doc/stable/reference/generated/numpy.ndarray.html and choose which we want to keep. Writing a wrapper for each will probably be no big task.

— Reply to this email directly, view it on GitHub https://github.com/TopoToolbox/pytopotoolbox/pull/19#issuecomment-2108660179, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RU3CYP52TD5KJGVOJM63ZCEJF3AVCNFSM6AAAAABHCMJIMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBYGY3DAMJXHE . You are receiving this because you commented.Message ID: @.***>

[wkearn]

I think that makes sense for now. @Teschl: Is there anything left to do here or should I go ahead and merge this PR?

[Teschl]

I added the str magic function so print(dem) is possible. And changed dem.zto demwhen plotting in the example. You should be able to merge it now.