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

Fixes #169, Thumbnails Updated #183

Merged
merged 4 commits into from
Sep 13, 2023
Merged

Conversation

devendragovil
Copy link
Contributor

Updated the thumbnails with tensor diagrams as suggested in #169

@ninamiolane
Copy link
Collaborator

@devendragovil Thanks!! And thanks for being that fast.

For some of them, the cropping is not very clean: parts of the neighboring tensor diagrams appear. I will send you the .svg version of the figure via email, so that you can crop only the diagram for each thumbnail.

Then we can merge!

@devendragovil
Copy link
Contributor Author

devendragovil commented Sep 7, 2023

@ninamiolane
I have detailed all the issues that I observed (while clipping screenshots) in issue #169 in a comment here.

Having SVG files would be awesome for quality and clarity.

Reproducing the previous comment below:

I have updated the thumbnails with tensor diagrams, I came across the following issues though:

  1. Same Tensor Diagram: There are multiple notebooks for (seemingly) same model. These include, examples:
    a. can_train_bis and can_train
    b. allset_train and allset_transformer_train
    c. hnhn_train_bis and hnhn_train
  2. Unavailable Model Tensor:
    a. Can't find any model named ccxn (for ccxn_train) in Fig 11. However the notebook is about Hajij (2020) so I used tensor diagram for cxn Hajij (2020)
    b. template_train notebook in Hypergraph (As of now thumbnail is a generic hypergraph)
  3. Multiple tensor diagrams: Some models have multiple tensor diagrams like CXN, Hajij (2020), HSN Hajij22b
  4. No clear screenshot: I was unable to take clear screenshots for some models due to encroachment of nearby tensor diagrams. @ninamiolane Wanted your opinion. Are the current thumbnails acceptable or should I also edit them? Example:
    a. CWN, Bodnar21b
    b. Dist2Cycle, Keros22
    c. SAN, Giusti22a
  5. Curious case of sccn_train: There is no sccn tensor diagram, however there is a scnn and the notebook of sccn seems to be pointing to the same author, year combination (Yang 22). However, there is another notebook with scnn. For now, sccn_train has thumbnail for scnn.
  6. There is scn2_train. Is it the same as scn?
  7. Scone_train_bis notebook seems to be referring to HSN Hajij 22, rather than the roddenberry paper. For now, both scone_train_bis and scone_train have the same tensor diagram

@mhajij
Copy link
Member

mhajij commented Sep 7, 2023

2. a. Can't find any model named ccxn (for ccxn_train) in Fig 11. However the notebook is about Hajij (2020) so I used tensor diagram for cxn Hajij (2020)

@ninamiolane , it might be a good idea to give @devendragovil the original CSV file to extract the TD from the file immediately.

@ninamiolane
Copy link
Collaborator

@devendragovil I've answered on the issue + shared the editable Fig 11 via email.
@mhajij definitely: done! ✔️

@ninamiolane ninamiolane marked this pull request as draft September 11, 2023 06:54
@ninamiolane
Copy link
Collaborator

@devendragovil converting to draft for now. Let me know when it's ready?

Side note: can_bis and scone_bis do not exist anymore. You can remove anything related to them. Thanks!

To make sure stuff works with updated changes
…editable file; Improving structure of Tutorials Page and Tutorials display; Adding README for tutorial contributors
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@devendragovil
Copy link
Contributor Author

The latest changes help resolve #169 and #162.

@ninamiolane Requesting your review.

@devendragovil devendragovil marked this pull request as ready for review September 13, 2023 00:21
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (9e11197) 97.29% compared to head (6ed555e) 97.29%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #183   +/-   ##
=======================================
  Coverage   97.29%   97.29%           
=======================================
  Files          57       57           
  Lines        2070     2070           
=======================================
  Hits         2014     2014           
  Misses         56       56           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Fantastic!! Thank you so much.

@ninamiolane ninamiolane merged commit 1ece602 into pyt-team:main Sep 13, 2023
6 checks passed
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.

3 participants