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

Moving loss and metrics to model.compile #340

Conversation

oliverholworthy
Copy link
Member

Updating Example Notebook to match changes made to the loss/metrics in NVIDIA-Merlin/models#431

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@nvidia-merlin-bot
Copy link
Contributor

Click to view CI Results
GitHub pull request #340 of commit c9f5788ed1b941ae47b036c7b878eb09410c8df2, no merge conflicts.
Running as SYSTEM
Setting status of c9f5788ed1b941ae47b036c7b878eb09410c8df2 to PENDING with url https://10.20.13.93:8080/job/merlin_merlin/109/console and message: 'Pending'
Using context: Jenkins
Building on master in workspace /var/jenkins_home/workspace/merlin_merlin
using credential systems-login
 > git rev-parse --is-inside-work-tree # timeout=10
Fetching changes from the remote Git repository
 > git config remote.origin.url https://github.com/NVIDIA-Merlin/Merlin # timeout=10
Fetching upstream changes from https://github.com/NVIDIA-Merlin/Merlin
 > git --version # timeout=10
using GIT_ASKPASS to set credentials login for merlin-systems
 > git fetch --tags --force --progress -- https://github.com/NVIDIA-Merlin/Merlin +refs/pull/340/*:refs/remotes/origin/pr/340/* # timeout=10
 > git rev-parse c9f5788ed1b941ae47b036c7b878eb09410c8df2^{commit} # timeout=10
Checking out Revision c9f5788ed1b941ae47b036c7b878eb09410c8df2 (detached)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f c9f5788ed1b941ae47b036c7b878eb09410c8df2 # timeout=10
Commit message: "Moving loss and metrics to model.compile"
 > git rev-list --no-walk 4e96462faf9a3739445ee71531f5761108423729 # timeout=10
[merlin_merlin] $ /bin/bash /tmp/jenkins4161815064205173175.sh
============================= test session starts ==============================
platform linux -- Python 3.8.10, pytest-7.1.2, pluggy-1.0.0
rootdir: /var/jenkins_home/workspace/merlin_merlin/merlin
plugins: anyio-3.5.0, xdist-2.5.0, forked-1.4.0, cov-3.0.0
collected 1 item

tests/unit/test_version.py . [100%]

============================== 1 passed in 0.02s ===============================
Performing Post build task...
Match found for : : True
Logical operation result is TRUE
Running script : #!/bin/bash
cd /var/jenkins_home/
CUDA_VISIBLE_DEVICES=1 python test_res_push.py "https://api.GitHub.com/repos/NVIDIA-Merlin/Merlin/issues/$ghprbPullId/comments" "/var/jenkins_home/jobs/$JOB_NAME/builds/$BUILD_NUMBER/log"
[merlin_merlin] $ /bin/bash /tmp/jenkins1636985879294788321.sh

@github-actions
Copy link

Documentation preview

https://nvidia-merlin.github.io/Merlin/review/pr-340

@karlhigley karlhigley requested a review from rnyak May 27, 2022 13:46
@karlhigley karlhigley added this to the Merlin 22.06 milestone May 27, 2022
@karlhigley karlhigley added bug Something isn't working breaking Breaking change enhancement New feature or request and removed bug Something isn't working labels May 27, 2022
@@ -391,7 +391,7 @@
" embedding_dim=64,\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Line #4.        loss="categorical_crossentropy", 

Instead of setting loss as a string, it'd be better to demonstrate how people can set from_logits=False/True arg and other args in the tf.keras.losses.CategoricalCrossentropy() ?

I'd prefer an example something like that to be similar to tf.keras notations:

model.compile(

  optimizer='adam', 
  run_eagerly=False, 
  loss=tf.keras.losses.CategoricalCrossentropy()
  metrics=[mm.RecallAt(10), mm.NDCGAt(10)]
)

what do you think?



Reply via ReviewNB

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 is just a change to fix the notebooks after underlying changes in Merlin Models broke them, but it's also worth considering how the examples could be improved once we make the fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants