Features/distances #694

VascoSch92 · 2023-09-12T15:40:50Z

Just a first sketch.

Let me know what do you think :-)

solegalli

Thank you so much for the contribution! This is looking really good.

I think the logic for the distance calculation is good. We need to make this transformer look like the other transformers that we have in the library.

Why don't you have a look at the class RelativeFeatures in the creation module, and try to incorporate that logic in this class as well? Mostly related to which parameters we need in the init, and which checks we normally do. And there you will see as well how you can import many premade bits of text for the documentation.

Let me know how you get along! Thank you!

feature_engine/creation/distance_features.py

solegalli · 2023-09-13T08:24:08Z

feature_engine/creation/distance_features.py

+        _check_param_drop_original(drop_original=drop_original)
+        self.drop_original = drop_original
+
+        self.variables = None


I don't think we need this parameter. I'd suggest using RelativeFeatures as template to model this class: https://github.com/VascoSch92/feature_engine/blob/e1e927625678ee73c5c3a9edcf79e955ff9c5e8e/feature_engine/creation/relative_features.py

variables is a parameter that we have in all transformers, so I would stick to this name instead of using coordinate_columns

In short, let's replace coordinate_columns by variables.

glevv · 2023-09-15T21:25:46Z

Is it possible to calculate Haversine distance using sklearn? It is quite fast and well optimized, reimplementing it seems like a not so good idea.

P.S. it could be quite interesting to add more measures of distance, for example Ruler distance

VascoSch92 · 2023-09-15T21:29:54Z

Is it possible to calculate Haversine distance using sklearn? It is quite fast and well optimized, reimplementing it seems like a not so good idea

Yes it is possible to compute the Haversine distance with sklearn. I was also thinking to use an apply and the Haversine distance method of Sklearn.

The question is: is it faster than vectorisation?

But I'm happy to change if it faster or If there is a faster method than mine ;-)

glevv · 2023-09-15T21:33:19Z

Is it possible to calculate Haversine distance using sklearn? It is quite fast and well optimized, reimplementing it seems like a not so good idea

Yes of course I know that. The question is: Can you vectorise it? it is faster than vectorisation?

I'm not sure I understand the question.
Scikit-learn implementation is vectorized by default

kylegilde · 2023-09-16T15:34:28Z

Is it possible to calculate Haversine distance using sklearn? It is quite fast and well optimized, reimplementing it seems like a not so good idea

Yes of course I know that. The question is: Can you vectorise it? it is faster than vectorisation?

I'm not sure I understand the question. Scikit-learn implementation is vectorized by default

I think the issue with the sklearn implementation is that it does a cartesian product between X and Y and yields a matrix.

We only need a pairwise calculation between X and Y that yields a vector.

glevv · 2023-09-16T21:45:36Z

np.diag(haversine_distances(X, Y) * R) would give you the vector you want

kylegilde · 2023-09-16T22:07:33Z

haversine_distances

I know that it is a simple way to code this, but from a time complexity perspective, it's not a great idea to use quadratic complexity when only linear complexity is needed.

glevv · 2023-09-17T13:18:19Z

haversine_distances

I know that it is a simple way to code this, but from a time complexity perspective, it's not a great idea to use quadratic complexity when only linear complexity is needed.

Yea, you are right, this way it will be better

solegalli · 2023-09-18T09:53:58Z

Is it possible to calculate Haversine distance using sklearn? It is quite fast and well optimized, reimplementing it seems like a not so good idea.

P.S. it could be quite interesting to add more measures of distance, for example Ruler distance

Hey @glevv thanks for the suggestion.

If I understood this blog correctly, it has 3 computations: euclidean, harvesine (the one we are trying to implement here) and a more complicated one that has a smaller error (vincenty's formula). Is this correct?

I'd suggest we stick to harvesine in this PR, and see if we create an issue to expand the class later with the Vincenty's. Is this formula commonly used? do we really need an error as small as 0.5mm for geo coordinates?

glevv · 2023-09-18T12:03:20Z

Is it possible to calculate Haversine distance using sklearn? It is quite fast and well optimized, reimplementing it seems like a not so good idea.
P.S. it could be quite interesting to add more measures of distance, for example Ruler distance

Hey @glevv thanks for the suggestion.

If I understood this blog correctly, it has 3 computations: euclidean, harvesine (the one we are trying to implement here) and a more complicated one that has a smaller error (vincenty's formula). Is this correct?

They are all measures of distance between two points on ellipsoid. There were no Vincenty formula, but it's quite heavy to compute. In this particular blog post they talked about two simpler and faster formulas (Cheap Ruler and FCC equation) but with higher error.

I'd suggest we stick to harvesine in this PR, and see if we create an issue to expand the class later with the Vincenty's. Is this formula commonly used? do we really need an error as small as 0.5mm for geo coordinates?

Ye, let's go with haversine only, not sure about Vincenty tho

VascoSch92 · 2024-02-03T17:35:44Z

Hey @solegalli
Sorry if I disappeared. I had a lot to do with work and life. I will try to give a look at this pull request next week ,-)

solegalli · 2024-02-05T08:22:36Z

No Problem at all @VascoSch92 . Same here.

I am doing some big changes to the correlation transformers, I think we could release a new version when i got those finished, hopefully during February.

It would be great if we can squeeze this transformer in that release 2. If you find the time, we look forward to your contribution :)

VascoSch92 · 2024-04-22T10:44:24Z

No Problem at all @VascoSch92 . Same here.

I am doing some big changes to the correlation transformers, I think we could release a new version when i got those finished, hopefully during February.

It would be great if we can squeeze this transformer in that release 2. If you find the time, we look forward to your contribution :)

Hey @solegalli :-)
is it time to give another try to this transformer? What do you think?

solegalli · 2024-04-22T10:51:49Z

Sure! Contributions are welcome any time :)

VascoSch92 · 2024-04-22T10:55:30Z

ok perfect. I will work on it.

VascoSch92 · 2024-05-09T21:10:00Z

Hey @solegalli
finally I have something.

I still need some guidance for some point:

From which classes should I extend? I'm extending from BaseNumericalTransformer, FitFromDictMixin and GetFeatureNamesOutMixin but I don't know if is a good idea
I have a fit method also if I'm not using it. Should I have it anyway or can I delete it?

solegalli

Hey @VascoSch92

This is looking good. We need to tidy the code a bit. In the init, we can only test that the user enters allowed inputs, and then assign them. We don't need extra functions/methods for this checks.

In the fit, we need to unpack the variables and check the allowed values.

If you could re-arrange that, I can then take another look.

It would be great if you could rebase main after you sync it to the latest version of the repo, because we made a lot of changes since this PR.

Thank you!

feature_engine/creation/distance_features.py

solegalli · 2024-08-30T10:14:50Z

feature_engine/creation/distance_features.py

+        return parameter
+
+    def _check_coordinate_columns(
+        self, columns: List[List[Union[str, int]]]


Is this list of lists? I think it is just a list.

feature_engine/creation/distance_features.py

VascoSch92 added 3 commits September 12, 2023 17:38

first version of distance features

2ac005e

test for distance features

de89663

correct types

e1e9276

solegalli linked an issue Sep 13, 2023 that may be closed by this pull request

Geo distance transformer #688

Open

solegalli reviewed Sep 13, 2023

View reviewed changes

VascoSch92 and others added 4 commits April 22, 2024 22:16

Merge branch 'feature-engine:main' into features/distances

3fb4fc5

update tests distance features

aeb3735

update distance features transformer

dc5dc60

add more tests

b318cce

VascoSch92 added 4 commits May 9, 2024 23:14

correct deprecated method

fb20eae

linting

da7bc6d

nit

4aa8570

fix type

0592e30

VascoSch92 mentioned this pull request Aug 25, 2024

transformer to apply mean normalization #763

Closed

solegalli reviewed Aug 30, 2024

View reviewed changes

VascoSch92 and others added 2 commits August 31, 2024 08:18

Merge branch 'feature-engine:main' into features/distances

1e8040d

first update after discussion

d425f72

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/distances #694

Features/distances #694

VascoSch92 commented Sep 12, 2023

solegalli left a comment

solegalli Sep 13, 2023

solegalli Aug 30, 2024

glevv commented Sep 15, 2023 •

edited

Loading

VascoSch92 commented Sep 15, 2023 •

edited

Loading

glevv commented Sep 15, 2023

kylegilde commented Sep 16, 2023

glevv commented Sep 16, 2023

kylegilde commented Sep 16, 2023

glevv commented Sep 17, 2023

solegalli commented Sep 18, 2023

glevv commented Sep 18, 2023

VascoSch92 commented Feb 3, 2024

solegalli commented Feb 5, 2024

VascoSch92 commented Apr 22, 2024

solegalli commented Apr 22, 2024

VascoSch92 commented Apr 22, 2024

VascoSch92 commented May 9, 2024

solegalli left a comment

solegalli Aug 30, 2024

Features/distances #694

Are you sure you want to change the base?

Features/distances #694

Conversation

VascoSch92 commented Sep 12, 2023

solegalli left a comment

Choose a reason for hiding this comment

solegalli Sep 13, 2023

Choose a reason for hiding this comment

solegalli Aug 30, 2024

Choose a reason for hiding this comment

glevv commented Sep 15, 2023 • edited Loading

VascoSch92 commented Sep 15, 2023 • edited Loading

glevv commented Sep 15, 2023

kylegilde commented Sep 16, 2023

glevv commented Sep 16, 2023

kylegilde commented Sep 16, 2023

glevv commented Sep 17, 2023

solegalli commented Sep 18, 2023

glevv commented Sep 18, 2023

VascoSch92 commented Feb 3, 2024

solegalli commented Feb 5, 2024

VascoSch92 commented Apr 22, 2024

solegalli commented Apr 22, 2024

VascoSch92 commented Apr 22, 2024

VascoSch92 commented May 9, 2024

solegalli left a comment

Choose a reason for hiding this comment

solegalli Aug 30, 2024

Choose a reason for hiding this comment

glevv commented Sep 15, 2023 •

edited

Loading

VascoSch92 commented Sep 15, 2023 •

edited

Loading