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.

Sign up for GitHub

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

Jump to bottom

Update case study: probabilistic matrix factorization #152

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Update case study: probabilistic matrix factorization #152

merged 6 commits into from
Oct 13, 2021

Conversation

chiral-carbon
Copy link
Collaborator

@chiral-carbon chiral-carbon commented May 3, 2021 •
edited
Loading

Addresses issue #58 and aims to advance it to best practices state.

General Updates

  • Uses the numpy.Generator module
  • Uses try... except clause to load data

Work in Progress

  • Using xarray to replace _norms function

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

OriolAbril
OriolAbril reviewed May 4, 2021
View reviewed changes
examples/case_studies/probabilistic_matrix_factorization.ipynb Outdated
@@ -16,22 +16,34 @@
"name": "stdout",
Copy link
Member

Choose a reason for hiding this comment

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

It should be arviz-darkgrid. It is based on seaborn-darkgrid but the color pallette is colorblind friendly.


Reply via ReviewNB

examples/case_studies/probabilistic_matrix_factorization.ipynb Outdated
@@ -16,22 +16,34 @@
"name": "stdout",
Copy link
Member

Choose a reason for hiding this comment

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

I would use something like:

csv_kwargs = dict(sep="\t", names=["userid", "itemid", "rating", "timestamp"])
try:
    data = pm.read_csv("../data/ml_100k_u.data", **csv_kwargs)
...

To try and make the cell easier to read and pleasant to the view. Otherwise between the kwargs and black it ends up taking a lot of vertical space without being easier to read.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah you are right, that's a good suggestion. thanks!

examples/case_studies/probabilistic_matrix_factorization.ipynb Outdated
@@ -16,22 +16,34 @@
"name": "stdout",
Copy link
Member

Choose a reason for hiding this comment

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

same aesthetic comment as above on the try except.


Reply via ReviewNB

@chiral-carbon chiral-carbon added tracker id Issues used as trackers in the notebook update project, do not close! outreachy2021 and removed tracker id Issues used as trackers in the notebook update project, do not close! labels Jun 14, 2021
@chiral-carbon chiral-carbon changed the title [WIP] Updating probabilistic matrix factorization for personlized recommendations to best practices state [WIP] Updating case study: probabilistic matrix factorization Aug 14, 2021
@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

Sayam753 commented on 2021-09-07T23:19:39Z
----------------------------------------------------------------

Line #6.        monitor = ("U", "V")

This is redundant

@chiral-carbon chiral-carbon changed the title [WIP] Updating case study: probabilistic matrix factorization Updating case study: probabilistic matrix factorization Sep 22, 2021
@chiral-carbon chiral-carbon changed the title Updating case study: probabilistic matrix factorization Update case study: probabilistic matrix factorization Sep 23, 2021
@chiral-carbon
Copy link
Collaborator Author

I think this is okay to be merged @OriolAbril

@OriolAbril
Copy link
Member

All citations use cite:p and are therefore within brackets. Some should be textual citations with text only, one also has double brackets, auto with the :p and manual ones, the manuals have to go.

In the section "Data", one code cell defines a ratings variable. It looks like it's not used anywhere.

There are several instances of [3] that look like they should be citations of A. Mnih and R. Salakhutdinov, “Probabilistic matrix factorization”

There is also a mention to the MovieLens 100k which should include a citation as per it's readme.

The convergence sections refers to rhat in math notation which is perfectly fine, but user lowercase r. The usual convention is using uppercase R.

@OriolAbril
Copy link
Member

There are several instances of [3] that look like they should be citations of A. Mnih and R. Salakhutdinov, “Probabilistic matrix factorization”

I still see these [3] things that look like references but are now not informative

@chiral-carbon
Copy link
Collaborator Author

@OriolAbril I think this can be merged now.

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T05:14:31Z
----------------------------------------------------------------

can you hide the output of the filelock cells below? with the cell tag to toggle things

@OriolAbril OriolAbril merged commit 445203c into pymc-devs:main Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OriolAbril OriolAbril OriolAbril approved these changes

Assignees
No one assigned
Labels
outreachy2021
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chiral-carbon @OriolAbril