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

Fix French notation for Tableau plots #35128

Merged
merged 5 commits into from
Mar 26, 2023
Merged

Conversation

thecaligarmo
Copy link
Contributor

@thecaligarmo thecaligarmo commented Feb 14, 2023

📚 Description

The change fixes a bug in which the plot of a Tableau was not properly showing French notation when trying to display the plot.

The fix in place allows the plot of a Tableau to accurately show the tableau in both French and English notation, as requested by the user.

This resolves open issue: #33998 which was imported from Trac

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

(As the issue is with plot and is a bug; no tests can cover the changes and documentation does not need changing)

⌛ Dependencies

@dimpase
Copy link
Member

dimpase commented Feb 14, 2023

don't you want to add tests to show this output in the different conventions? maybe using sphinx_plot(), e.g. like in
src/sage/combinat/path_tableaux/frieze.py

@thecaligarmo
Copy link
Contributor Author

don't you want to add tests to show this output in the different conventions? maybe using sphinx_plot(), e.g. like in src/sage/combinat/path_tableaux/frieze.py

Good idea =D I've included a couple of plots to show the difference. I think there's no way to test them though right?

@dimpase
Copy link
Member

dimpase commented Feb 14, 2023

looking at the results (actually, right here, after build-docs CI step is done would be good enough. Also, the edicational value of seeing the difference between French and English in the docs is a plus.

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

can we have these examples slightly less non-generic? (It would also take away the fear that in general it might not work)

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2023

Codecov Report

Base: 88.59% // Head: 88.58% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (ef66477) compared to base (fbb4127).
Patch coverage: 87.50% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35128      +/-   ##
===========================================
- Coverage    88.59%   88.58%   -0.01%     
===========================================
  Files         2140     2140              
  Lines       396961   396964       +3     
===========================================
- Hits        351677   351655      -22     
- Misses       45284    45309      +25     
Impacted Files Coverage Δ
src/sage/combinat/tableau.py 96.16% <87.50%> (-0.05%) ⬇️
src/sage/interfaces/qsieve.py 71.30% <0.00%> (-2.61%) ⬇️
src/sage/cpython/_py2_random.py 74.79% <0.00%> (-1.66%) ⬇️
src/sage/modular/hecke/algebra.py 94.65% <0.00%> (-1.07%) ⬇️
src/sage/combinat/posets/poset_examples.py 87.67% <0.00%> (-1.01%) ⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py 94.09% <0.00%> (-0.79%) ⬇️
...e/combinat/cluster_algebra_quiver/mutation_type.py 52.37% <0.00%> (-0.60%) ⬇️
src/sage/combinat/constellation.py 91.18% <0.00%> (-0.41%) ⬇️
src/sage/schemes/elliptic_curves/cardinality.py 87.00% <0.00%> (-0.40%) ⬇️
src/sage/modular/overconvergent/hecke_series.py 98.76% <0.00%> (-0.31%) ⬇️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dimpase dimpase self-requested a review February 15, 2023 11:12
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: dab892d

@vbraun vbraun merged commit b80d04e into sagemath:develop Mar 26, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants