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

Implement symmetry classes of plane partitions #35037

Conversation

tscrim
Copy link
Collaborator

@tscrim tscrim commented Feb 9, 2023

Implement symmetric classes on plane partitions #28244

… size of box of parent (if defined), and largest bounding box otherwise
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Base: 88.59% // Head: 88.59% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (9ff10da) compared to base (84eebf0).
Patch coverage: 96.25% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #35037    +/-   ##
=========================================
  Coverage    88.59%   88.59%            
=========================================
  Files         2140     2140            
  Lines       396961   397657   +696     
=========================================
+ Hits        351671   352290   +619     
- Misses       45290    45367    +77     
Impacted Files Coverage Δ
src/sage/schemes/toric/sheaf/klyachko.py 92.43% <ø> (ø)
src/sage/combinat/plane_partition.py 96.86% <96.25%> (-0.67%) ⬇️
...e/combinat/cluster_algebra_quiver/mutation_type.py 47.50% <0.00%> (-2.74%) ⬇️
src/sage/groups/additive_abelian/qmodnz.py 91.48% <0.00%> (-2.13%) ⬇️
src/sage/doctest/forker.py 80.24% <0.00%> (-1.48%) ⬇️
src/sage/homology/matrix_utils.py 87.15% <0.00%> (-0.92%) ⬇️
src/sage/groups/generic.py 88.34% <0.00%> (-0.68%) ⬇️
...sage/geometry/hyperbolic_space/hyperbolic_model.py 88.95% <0.00%> (-0.62%) ⬇️
src/sage/graphs/graph_plot.py 84.33% <0.00%> (-0.57%) ⬇️
... and 21 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.

@tscrim tscrim requested a review from fchapoton February 14, 2023 01:34
@fchapoton
Copy link
Contributor

the seems to be a problem with NewType somewhere, see the build & test

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 14, 2023

the seems to be a problem with NewType somewhere, see the build & test

Are we sure that is a real error? It is something with pyright that I don't really know. I do not know what to even test to try and reproduce locally.

@fchapoton
Copy link
Contributor

The second argument of NewType must be a class, not a string. Either remove all annotations with PP (namely --> PP), or try using PP = NewType('PP', PlanePartition). Not sure what's best and what will work.

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 15, 2023

Okay, done. Let's try this again.

@github-actions
Copy link

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

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 17, 2023

All tests now pass. Thanks @fchapoton

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 28, 2023

@kevindilks @jessicapalencia @sagetrac-jangsookim Could one of you finish the review for this? It is just checking my last commits.

@fchapoton
Copy link
Contributor

one import of ZZ zhould be changed

change the import of ZZ
@tscrim
Copy link
Collaborator Author

tscrim commented Mar 28, 2023

@fchapoton Thank you. I wasn't aware it was easy to push review changes like that. (In fact, it is somewhat worrisome the default allows such pushes...) Is there anything else that needs to be done?

@fchapoton
Copy link
Contributor

I guess I may have been given some super-powers. Maybe I should have not done that, but it was such a simple change..

I think this is ready.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 28, 2023

I would be a bit surprised about that since you pushed to the branch on my fork. It might be a normal part of working on GH (I don't recall seeing anything about this on the transition/workflow guide).

Then are you approving the PR? (I can't do this since it is coming from my fork.)

@fchapoton
Copy link
Contributor

when creating a pull request, there is a box to tick where you can allow changes on your own fork by the managers of the main repo.

Yes, let me approve the thing here. I would rather squash all these commits, but it may complicated, no idea about what would happen with the new process..

Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

good to go, in my opinion

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 28, 2023

Ah, I see. It is a bit annoying that it is somewhat limited as well. There are definitely times I wish I could just push a change rather than do the inline comments (and I really do not want to fragment stuff with a separate PR). Well...it it what it is.

Thank you.

Squashing them might be complicated because of the different versions that this has been built with. It probably would be good, but I think something of squashing-all-commits-per-PR should be decided on sage-devel since it can have impacts on bisections and inspecting the change history. (Although this case might still benefit from it.) Anyways, perhaps for another time.

@vbraun vbraun merged commit e380758 into sagemath:develop Apr 1, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
@tscrim tscrim deleted the public/combinat/symmetry_classes_plane_partitions-28244 branch April 28, 2023 05:31
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.

6 participants