Opened 6 years ago
Closed 6 years ago
#22160 closed enhancement (fixed)
Use centralized references in ClusterSeed
Reported by:  Salvatore Stella  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  documentation  Keywords:  documentation, days82 
Cc:  Gregg Musiker, Dylan Rupel, Christian Stump, Travis Scrimshaw, Emily Gunawan  Merged in:  
Authors:  Salvatore Stella  Reviewers:  Emily Gunawan, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  b787c1c (Commits, GitHub, GitLab)  Commit:  b787c1cfa8303a5302fe3998eaa270a2126ac9fe 
Dependencies:  Stopgaps: 
Description (last modified by )
While reviewing #21254 egunawan found a double reference due to the fact that ClusterSeed
still has references defined locally.
This ticket fixes the issue.
Change History (28)
comment:1 Changed 6 years ago by
Branch:  → public/ticket/22160 

Commit:  → 04f4244474d651adc9eacfa1303bba31e591900f 
comment:2 Changed 6 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
comment:3 Changed 6 years ago by
Commit:  04f4244474d651adc9eacfa1303bba31e591900f → 175b53aa7054b9483f12747da3b27dc4b8ee32af 

Branch pushed to git repo; I updated commit sha1. New commits:
175b53a  I can\'t even remember the lexicographic order!

comment:4 Changed 6 years ago by
Keywords:  days82 added 

Status:  needs_review → needs_info 
There are several more local references in cluster_seed.py. We should move them to index.rst as well?
comment:7 Changed 6 years ago by
Commit:  175b53aa7054b9483f12747da3b27dc4b8ee32af → 976b736920f2ddbeb473440d7e71d304e0d885fa 

Branch pushed to git repo; I updated commit sha1. New commits:
976b736  Moved all references

comment:8 Changed 6 years ago by
Status:  needs_info → needs_review 

comment:9 Changed 6 years ago by
Commit:  976b736920f2ddbeb473440d7e71d304e0d885fa → 474750e709dfb6d23b5eb99d856690fa4e74c4fb 

Branch pushed to git repo; I updated commit sha1. New commits:
474750e  Added missing references

comment:10 Changed 6 years ago by
There is a typo but I am recompiling on a different branch and I can't fix it right now. I should be able to mend the problem as soon as the documentation is done building.
comment:11 Changed 6 years ago by
Commit:  474750e709dfb6d23b5eb99d856690fa4e74c4fb → b3ced177ba29c0b81c3bc0b6071fdc7d5e27ac9d 

Branch pushed to git repo; I updated commit sha1. New commits:
b3ced17  Fix typo

comment:13 Changed 6 years ago by
Component:  PLEASE CHANGE → documentation 

Reviewers:  → Travis Scrimshaw 
Status:  needs_review → positive_review 
LGTM.
comment:14 Changed 6 years ago by
Milestone:  sage7.5 → sage7.6 

Reviewers:  Travis Scrimshaw → Emily Gunawan, Travis Scrimshaw 
comment:15 Changed 6 years ago by
Status:  positive_review → needs_info 

There are other references to actual websites (or arxiv numbers) in cluster_seed.py and other parts of the cluster_algebra_quiver folder. This will not cause issue for Sage, but, since this ticket was already opened, should we move those local references as well? If you don't think we should do that, you can set this back to positive_review.
comment:16 Changed 6 years ago by
Commit:  b3ced177ba29c0b81c3bc0b6071fdc7d5e27ac9d → 29cc90a37869d74e20f5f5c05a4c04a5812b3b2c 

comment:17 Changed 6 years ago by
Status:  needs_info → needs_review 

I now changed also all the references in every file under the folder cluster_algebra_quiver. They were in a messy variety of different styles.
comment:18 Changed 6 years ago by
Status:  needs_review → needs_work 

We should be good and make sure those references have full bib information. For example, I suspect the Speyer reference has been published in a journal.
comment:19 Changed 6 years ago by
I already did that. I could not find better references for [Spe2013]_ nor for [Ke2008]_. I did fix many of the others.
comment:20 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:21 Changed 6 years ago by
Okay, thanks. I will let Emily set it back to a positive review of there is nothing else.
comment:22 Changed 6 years ago by
Commit:  29cc90a37869d74e20f5f5c05a4c04a5812b3b2c → c71e227f6808318cc36feab49c2b0ed1ad8b009d 

Branch pushed to git repo; I updated commit sha1. New commits:
c71e227  22160: fix broken reference to see sage.graphs.generic_graph.GenericGraph.canonical_label()

comment:23 Changed 6 years ago by
Salvatore, you can set this to positive_review if you're OK with the change I just did (fixing a broken reference to GenericGraph?.canonical_label).
comment:26 Changed 6 years ago by
Commit:  c71e227f6808318cc36feab49c2b0ed1ad8b009d → b787c1cfa8303a5302fe3998eaa270a2126ac9fe 

Branch pushed to git repo; I updated commit sha1. New commits:
49dc8b8  Use centralized references

318d348  I can\'t even remember the lexicographic order!

71149f7  Moved all references

dbf7100  Added missing references

591fbc0  Fix typo

2b395ee  Changed all the remaining references in the cluster_algebra_quiver folder

4a0c541  Minor fixes

be62a45  22160: fix broken reference to see sage.graphs.generic_graph.GenericGraph.canonical_label()

b787c1c  Fixed merge conflict

comment:27 Changed 6 years ago by
Status:  needs_work → positive_review 

I rebased the branch to the current develop. This and some other ticket introduced two new references in the same place generating the conflict. It should be good now.
comment:28 Changed 6 years ago by
Branch:  public/ticket/22160 → b787c1cfa8303a5302fe3998eaa270a2126ac9fe 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Use centralized references