Opened 6 years ago
Closed 6 years ago
#15391 closed enhancement (fixed)
Implementing the Foata bijection on permutations
Reported by: | MariaMonks | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.1 |
Component: | combinatorics | Keywords: | days54, combinat, findstat02 |
Cc: | jessicapalencia, tscrim, sage-combinat | Merged in: | |
Authors: | Maria Monks | Reviewers: | Jessica Striker, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | public/combinat/permutation/foata-15391 (Commits) | Commit: | 5a60319ced9c1921892f2b4235a4439e5a84672e |
Dependencies: | Stopgaps: |
Description (last modified by )
Adds a method for applying the Foata bijection to a permutation.
Change History (19)
comment:1 Changed 6 years ago by
- Branch set to public/combinat/permutation/foata-15391
- Cc tscrim sage-combinat added
- Commit set to b2ba47e743d882999d716f4ac810916eda534413
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Keywords findstat02 added
comment:3 Changed 6 years ago by
- Commit changed from b2ba47e743d882999d716f4ac810916eda534413 to be7baf7a74abaa63efb165a429d3699535aebb43
Branch pushed to git repo; I updated commit sha1. New commits:
be7baf7 | Doc test edits |
comment:4 Changed 6 years ago by
- Reviewers set to Jessica Striker
Hi Maria, The code looks good. A couple of the doc tests failed, though. After you edit the permutation.py file, you can run the doc tests by typing from the command line: sage -bt permutation.py
(the -b builds sage and the -t runs the doc tests in the file you specify)
This rebuilds sage with your code inserted and runs the doc tests. I fixed the syntax on your doc tests so that they work and pushed the changes to trac. You can see the diff by clicking on (commits) in the branch field above. Or you can pull the changes to your local machine to test them out. If you are happy with my changes, you may set the ticket to positive review.
comment:5 Changed 6 years ago by
- Commit changed from be7baf7a74abaa63efb165a429d3699535aebb43 to 6906e12fa9f32b19d3f8e45e82cc2d7270972a27
Branch pushed to git repo; I updated commit sha1. New commits:
6906e12 | formatting changes (we aren't saving the environment) |
comment:6 Changed 6 years ago by
- Commit changed from 6906e12fa9f32b19d3f8e45e82cc2d7270972a27 to 76b82a84d76a340d242cf4e6791dc763938f8d51
comment:7 Changed 6 years ago by
Hi Maria,
nice work. If you're fine with my changes, set this to positive_review. Unfortunately, a (trivial, I hope) conflict resolution might become necessary once my #15174 is merged into Sage.
Greets,
Darij
comment:8 Changed 6 years ago by
- Commit changed from 76b82a84d76a340d242cf4e6791dc763938f8d51 to 5a60319ced9c1921892f2b4235a4439e5a84672e
comment:9 Changed 6 years ago by
- Reviewers changed from Jessica Striker to Jessica Striker, Travis Scrimshaw
I've updated it to the latest 5.13.beta3
, simplified the logic and made some additional tweaks. I'm happy with it, so it's up to someone else to check my changes and set a positive review.
comment:10 Changed 6 years ago by
Hi all,
Sorry, I'm new to this. How do I see the changed file all in one place? Do I pull something from the repository? I just want to check it over before setting it to positive review. Thanks for all your tweaks!
Maria
comment:11 Changed 6 years ago by
Hi Maria,
I was just looking at it now too. Yes, you should switch to your local branch and pull the changes from the trac server. I believe the commands are:
git fetch origin
pull --ff-only
Jessica
comment:12 follow-up: ↓ 13 Changed 6 years ago by
After you pull, you can look at the new code, but if you want to test the features in Sage, you need to build sage:
sage -bt permutation.py
But for me, it's taking a long time to build, probably because it's upgrading to a new beta version. (Is this to be expected, Travis?)
Anyway, once you're happy with it, you can set it to positive review.
comment:13 in reply to: ↑ 12 Changed 6 years ago by
Replying to jessicapalencia:
But for me, it's taking a long time to build, probably because it's upgrading to a new beta version. (Is this to be expected, Travis?)
Yes, it's expected. The recent bump changes some low-level .pxi
or similar such file that everything depends on, so a big recompile occurs. I got annoyed by this and merged the latest beta version into all my branches. Although this should improve significantly with #15430 (and ccache I think...) in the sense that we shouldn't need to do the recompile, or at least it will be significantly sped up, when changing between branches which change cython files.
comment:14 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:15 Changed 6 years ago by
Ok, all the changes look good, and I set it to positive review. (And yes, I also had to leave sage to rebuild overnight.) What happens next?
comment:16 Changed 6 years ago by
- Description modified (diff)
- Milestone changed from sage-5.13 to sage-6.0
comment:17 Changed 6 years ago by
- Description modified (diff)
In the case of no conflict, it gets merged into Sage 6.0 more or less automatically (unless the patchbot finds errors in the patch).
If some conflict arises (this might happen due to #15174 and other mercurial stuff that will probably get merged before this one), the release manager will reset this ticket to needs_work, and someone will have to make a merge commit to bring this patch up to date. "Someone" can be any of us, as resolving this kind of conflict (bugfixes in old methods vs. introduction of a new method) should be very easy. (Maybe the release manager will do it himself.)
Good work, Maria! (Already using the method...)
comment:18 Changed 6 years ago by
- Milestone changed from sage-6.0 to sage-6.1
comment:19 Changed 6 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
New commits: