[MRG] MAINT: remove variables not needed to store #159

wdevazelhes · 2019-01-23T13:54:17Z

Fixes #134

wdevazelhes · 2019-01-23T13:55:29Z

metric_learn/itml.py

+      ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+      ``d``, with ``bounds=[pos, neg]``, and ``d`` the learned distance. If
+      not provided at initialization, these are the ones derived at train
+      time.


I am not familiar with ITML but the above is based on what I understood of it. Feel free to tell me if I'm wrong

As currently implemented, bounds_ should be an ndarray-like of shape (2,).

We should probably update the code to accept list or tuple types for the bounds as well.

"these are the ones" -> "these are"

wdevazelhes · 2019-01-23T13:57:12Z

metric_learn/itml.py

+        all given pairs of similar points ``a`` and ``b``, and
+        ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+        ``d``, with ``bounds=[pos, neg]``, and ``d`` the learned distance.
+        If not provided at initialization, these will be derived at train time.


I also updated the description of the argument in fit (bounds), to match the one of self.bounds_, and to be more detailed. Same comment: feel free to tell me if I'm wrong

perimosocordiae · 2019-01-23T15:10:10Z

metric_learn/itml.py

+      ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+      ``d``, with ``bounds=[pos, neg]``, and ``d`` the learned distance. If
+      not provided at initialization, these are the ones derived at train
+      time.


As currently implemented, bounds_ should be an ndarray-like of shape (2,).

We should probably update the code to accept list or tuple types for the bounds as well.

perimosocordiae · 2019-01-23T15:11:15Z

metric_learn/itml.py

+      ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+      ``d``, with ``bounds=[pos, neg]``, and ``d`` the learned distance. If
+      not provided at initialization, these are the ones derived at train
+      time.


"these are the ones" -> "these are"

perimosocordiae · 2019-01-23T15:12:31Z

metric_learn/itml.py

+      time.
+
+  n_iter_ : `int`
+      The number of iterations the solver has ran.


"has ran" -> "has run"

As currently implemented, bounds_ should be an ndarray-like of shape (2,).

We should probably update the code to accept list or tuple types for the bounds as well.

That's right, I forgot about that. For now I'll just change the docstring, but I'll open an issue to get this thing straight

"these are the ones" -> "these are"

Thanks, will do

"has ran" -> "has run"

That's right, thanks

wdevazelhes · 2019-01-24T09:58:05Z

metric_learn/itml.py

@@ -181,16 +181,16 @@ class ITML_Supervised(_BaseITML, TransformerMixin):

  Attributes
  ----------
-  bounds_ : `list` of two numbers
+  bounds_ : array-like, shape=(2,)
      Bounds on similarity, aside slack variables, s.t. ``d(a, b) < pos`` for
      all given pairs of similar points ``a`` and ``b``, and
      ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
      ``d``, with ``bounds=[pos, neg]``, and ``d`` the learned distance. If


I keep saying bounds=[pos, neg] here because it can be still be a list here, and I think this is more general than bounds=array([pos, neg]) which would make think more of a numpy array only

bellet · 2019-01-25T13:13:51Z

metric_learn/itml.py

+      all given pairs of similar points ``a`` and ``b``, and
+      ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+      ``d``, with ``bounds=[ pos, neg]``, and ``d`` the learned distance. If
+      not provided at initialization, these are derived at train


maybe it would be nice to explain how it is done then: bounds_[0] and bounds_[1] are set to the 5th and 95th percentile of the pairwise distances among all points available at training?
for supervised version, of all points in the training data X ; for weakly supervised, of all points present in pairs

Yes I agree it's more precise

bellet · 2019-01-25T13:13:58Z

metric_learn/itml.py

+        all given pairs of similar points ``a`` and ``b``, and
+        ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+        ``d``, with ``bounds=[pos, neg]``, and ``d`` the learned distance.
+        If not provided at initialization, these will be derived at train time.


bellet · 2019-01-25T13:15:36Z

metric_learn/itml.py

+      ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+      ``d``, with ``bounds=[pos, neg]``, and ``d`` the learned distance. If
+      not provided at initialization, these are derived at train
+      time.


bellet · 2019-01-25T13:15:45Z

metric_learn/itml.py

+      all given pairs of similar points ``a`` and ``b``, and ``d(c, d) > neg``
+      for all given pairs of dissimilar points ``c`` and ``d``, with
+      ``bounds=[pos, neg]``, and ``d`` the learned distance. If not provided at
+      initialization, these will be derived at train time.


wdevazelhes · 2019-01-29T10:43:16Z

Regarding bounds, I also noticed that for the supervised version it has to be given at initalization, and for the weakly supervised version it has to be put at fit. I think we should uniformize that. Since it is a data dependent parameter (indeed the default setting is deduced from the data), I think we should put them in fit. Since this is out of the scope of this PR and it should have a deprecation warning (which adds again code in this PR), I'll open another PR for that, and for this one I'll let the parameter where they are.

wdevazelhes · 2019-01-29T11:10:10Z

I adressed all the comments, so we should be good to merge this one

bellet · 2019-01-29T12:30:36Z

metric_learn/itml.py

+      Bounds on similarity, aside slack variables, s.t. ``d(a, b) < pos`` for
+      all given pairs of similar points ``a`` and ``b``, and
+      ``d(c, d) > neg`` for all given pairs of dissimilar points ``c`` and
+      ``d``, with ``bounds=[ pos, neg]``, and ``d`` the learned distance. If


My suggestion was to get rid of bounds=[ pos, neg] altogether and replace pos and neg in the inequalities by bounds_[0] and bounds_[1]

Ah yes I agree it's even clearer, done

# Conflicts: # metric_learn/sdml.py

* MAINT: remove variables not needed to store * Address review #159 (review) * DOC: add more precise docstring * API: put parameter in fit, deprecate it in init, and also change previous deprecation tests names * Change remaining test names

MAINT: remove variables not needed to store

94ec950

wdevazelhes commented Jan 23, 2019

View reviewed changes

wdevazelhes requested review from perimosocordiae and bellet January 23, 2019 13:58

wdevazelhes mentioned this pull request Jan 23, 2019

Avoid storing any unnecessary variable after fit #134

Closed

perimosocordiae approved these changes Jan 23, 2019

View reviewed changes

wdevazelhes mentioned this pull request Jan 24, 2019

improve bounds argument and bounds_ attribute in ITML #161

Closed

Address review scikit-learn-contrib#159 (review)

2d276c8

wdevazelhes commented Jan 24, 2019

View reviewed changes

wdevazelhes changed the title ~~MAINT: remove variables not needed to store~~ [MRG] MAINT: remove variables not needed to store Jan 24, 2019

bellet approved these changes Jan 25, 2019

View reviewed changes

DOC: add more precise docstring

41227a6

wdevazelhes mentioned this pull request Jan 29, 2019

[MRG] change bounds parameter of ITML_Supervised from init to fit #163

Merged

bellet approved these changes Jan 29, 2019

View reviewed changes

William de Vazelhes added 2 commits January 29, 2019 13:49

Merge branch 'master' into maint/dont_store_variables

1dda273

# Conflicts: # metric_learn/sdml.py

DOC: make description clearer

bf47d8f

bellet merged commit b336eba into scikit-learn-contrib:master Jan 29, 2019

wdevazelhes deleted the maint/dont_store_variables branch January 29, 2019 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] MAINT: remove variables not needed to store #159

[MRG] MAINT: remove variables not needed to store #159

wdevazelhes commented Jan 23, 2019

wdevazelhes Jan 23, 2019

perimosocordiae Jan 23, 2019

perimosocordiae Jan 23, 2019

wdevazelhes Jan 23, 2019

perimosocordiae Jan 23, 2019

perimosocordiae Jan 23, 2019

perimosocordiae Jan 23, 2019

wdevazelhes Jan 24, 2019

wdevazelhes Jan 24, 2019

bellet Jan 25, 2019

wdevazelhes Jan 29, 2019

bellet Jan 25, 2019

wdevazelhes Jan 29, 2019

bellet Jan 25, 2019

wdevazelhes Jan 29, 2019

bellet Jan 25, 2019

wdevazelhes Jan 29, 2019

wdevazelhes commented Jan 29, 2019

wdevazelhes commented Jan 29, 2019

bellet Jan 29, 2019

wdevazelhes Jan 29, 2019

[MRG] MAINT: remove variables not needed to store #159

[MRG] MAINT: remove variables not needed to store #159

Conversation

wdevazelhes commented Jan 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdevazelhes commented Jan 29, 2019

wdevazelhes commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment