Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare new version #232

Merged

Conversation

wdevazelhes
Copy link
Member

This PR is to prepare the new release and discuss the process/timeline

@bellet
Copy link
Member

bellet commented Jul 4, 2019

@perimosocordiae @terrytangyuan your input is welcome! Our plan is to release soon but maybe there are some points you would like to see investigated a bit further before the release? Potentially related to the last few PRs that we had to merge rather quickly (we were organizing a hands-on session on metric-learn this morning in our research group and we wanted everything to be merged)

In any case we need to carefully write the change log listing the numerous changes since the last release!

@bellet bellet added this to the v0.5.0 milestone Jul 4, 2019
@wdevazelhes
Copy link
Member Author

I started to update the changelog https://github.com/metric-learn/metric-learn/releases, I still need to describe the main API changes of WeaklySupervised algorithms (I think I'll point to the documentation too so that it's simpler), let me know what you think otherwise

@bellet
Copy link
Member

bellet commented Jul 5, 2019

Nice, I made a few small edits. For weakly supervised algorithms, I think it would be nice to describe at a high level the main changes and their concrete implications (model selection / hyperparameter tuning, scoring, etc). Pointing to the doc for details is fine.

@terrytangyuan
Copy link
Member

This is great! I agree it would be good to have a new release to include the changes so far.

@bellet
Copy link
Member

bellet commented Jul 5, 2019

Some points that came up during the hands-on session:

  • There seemed to be a compatibility problem with the Graphical Lasso import and sklearn v0.20
  • Some methods (such as check_preprocessor) should probably made private
  • The results of NCA (and maybe other estimators) with random init change even when setting the random_state

@wdevazelhes
Copy link
Member Author

  • I tried to reproduce the bug with sklearn 0.20.3 but I can't manage to do it. I'll investigate this further
  • For the check_preprocessor, I agree, I'll open a PR for that and check that there are no other case like this
  • For the random behaviour, indeed, I forgot to add the random_state when initializing the matrix... I'll add it in a PR and add a test that all our estimator that have either a 'init' or a 'prior' are deterministic when setting the random_seed. (I would test that any estimator with a random_seed parameter is deterministic for any value of the others parameters, but unfortunately there's no systematic way to enumerate all the values that the parameters can take for now)

@wdevazelhes
Copy link
Member Author

I just opened the 2 PRs, and updated the draft for the release, let me know what you think
I just need to investigate the problem with sklearn 0.20

@bellet
Copy link
Member

bellet commented Jul 5, 2019

Looks good, I did minor edits. Let's sort out this sklearn 0.20 thing while we wait for feedback from @perimosocordiae :-)

@bellet
Copy link
Member

bellet commented Jul 6, 2019

So the sklearn 0.20 thing was a false alarm, it turns out the version for which the problem occurs is 0.19, which is fine since we require 0.20

@perimosocordiae
Copy link
Contributor

I think we're just about ready to release! I want to take a look at the current state of the docs one more time before we cut, but code-wise we should be good.

doc/conf.py Outdated
@@ -22,8 +22,8 @@
project = u'metric-learn'
copyright = u'2015-2018, CJ Carey and Yuan Tang'
author = u'CJ Carey and Yuan Tang'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line needs some updating as well!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, done, for the order I thought [original developers | my supervisors | me]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put yourself before Nathalie and I!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allright, done ;)

@bellet
Copy link
Member

bellet commented Jul 10, 2019

I think we're just about ready to release! I want to take a look at the current state of the docs one more time before we cut, but code-wise we should be good.

Sounds good, feel free to open an issue/PR or let us know if everything is good

@wdevazelhes
Copy link
Member Author

@perimosocordiae @terrytangyuan @bellet @nvauquie, I guess we are ready for the release, unless you have any other comment ? For the guidelines I'll follow what we did last time in #71

@bellet
Copy link
Member

bellet commented Jul 18, 2019

Otherwise LGTM :-)

@terrytangyuan
Copy link
Member

Looks great! Thanks.

@perimosocordiae perimosocordiae merged commit 57086e9 into scikit-learn-contrib:master Jul 18, 2019
@perimosocordiae
Copy link
Contributor

Merged.

@wdevazelhes
Copy link
Member Author

Great ! I'll lauch the release

@wdevazelhes
Copy link
Member Author

Done ! The docs are not yet updated though... Let's wait until tomorrow in case.. (I updated the gh-pages branch with a doc built locally but github does not seem to updated the changes)

@wdevazelhes
Copy link
Member Author

I did a wrong process to push the docs in fact, now I remembered and it works ! I guess the release is finalized 🎉 🎉

@bellet
Copy link
Member

bellet commented Jul 19, 2019

Congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants