Opened 6 years ago
Closed 6 years ago
#17191 closed enhancement (fixed)
Poset: change rank dict to rank array
Reported by: | jmantysalo | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-6.4 |
Component: | combinatorics | Keywords: | |
Cc: | Merged in: | ||
Authors: | Nathann Cohen | Reviewers: | Jori Mäntysalo |
Report Upstream: | N/A | Work issues: | |
Branch: | cc8fd43 (Commits, GitHub, GitLab) | Commit: | cc8fd43ab6d177faa74fd90b9979d5ff17da4bb7 |
Dependencies: | Stopgaps: |
Description
On hasse_diagram.py
there is now _rank_dict
whose keys are just numbers 0,...,n
. It can be converted to array, and it will be slightly faster then.
Change History (12)
comment:1 Changed 6 years ago by
- Branch set to u/ncohen/17191
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Commit set to 08727b253f8ecc25285f2e0c01a21424868bcf63
comment:3 Changed 6 years ago by
- Reviewers set to Jori Mäntysalo
- Status changed from needs_review to needs_work
About
return lambda x:self._rank[x] # the rank function is just the getitem of the dict
The comment string should be changed, "dict" -> "list". Also why bother with lambda function, because lists also has a __getitem__
?
Line
return [rank_fcn[i] for i in range(self.order())]
seems strange --- isn't that just same as return rank_fcn
or return copy(rank_fcn)
? Or maybe I didn't understand this.
comment:4 follow-up: ↓ 7 Changed 6 years ago by
Yooo !
return lambda x:self._rank[x] # the rank function is just the getitem of the dictThe comment string should be changed, "dict" -> "list". Also why bother with lambda function, because lists also has a
__getitem__
?
You are right.
return [rank_fcn[i] for i in range(self.order())]seems strange --- isn't that just same as
return rank_fcn
orreturn copy(rank_fcn)
? Or maybe I didn't understand this.
Because rank_fcn
is a dictionary, not a list, and because it should be a dictionary in this function (because of lines like 'if x in rank_fcn').
Nathann
comment:5 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:6 Changed 6 years ago by
- Commit changed from 08727b253f8ecc25285f2e0c01a21424868bcf63 to 45fc474b60ea8de4d76f4aa98c64b9b2765743fe
Branch pushed to git repo; I updated commit sha1. New commits:
45fc474 | trac #17191: Reviewer's comments
|
comment:7 in reply to: ↑ 4 ; follow-up: ↓ 8 Changed 6 years ago by
- Milestone changed from sage-wishlist to sage-6.4
- Status changed from needs_review to positive_review
Replying to ncohen:
return [rank_fcn[i] for i in range(self.order())]seems strange --- isn't that just same as
return rank_fcn
orreturn copy(rank_fcn)
? Or maybe I didn't understand this.Because
rank_fcn
is a dictionary, not a list, and because it should be a dictionary in this function (because of lines like 'if x in rank_fcn').
Ah, true. This could also start with rank_fcn=[None]*self.order()
instead of rank_fcn={}
, but the current code seems to be cleaner.
I think this is ready for production, so I also changed milestone with status.
comment:8 in reply to: ↑ 7 Changed 6 years ago by
Yo !
Ah, true. This could also start with
rank_fcn=[None]*self.order()
instead ofrank_fcn={}
, but the current code seems to be cleaner.
Right again. And I could have turned the 'if x in rank_fcn' into 'if rank_fcn[x] is Null'... Stupid me.
I think this is ready for production, so I also changed milestone with status.
I hope that you do not mind: I am going to add a commit in a second to do that properly with a list in the function too. Not very important, but worth doing anyway.
Nathann
comment:9 Changed 6 years ago by
- Commit changed from 45fc474b60ea8de4d76f4aa98c64b9b2765743fe to cc8fd43ab6d177faa74fd90b9979d5ff17da4bb7
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
cc8fd43 | trac #17191: Doing the job properly
|
comment:11 Changed 6 years ago by
Thanks !
Nathann
comment:12 Changed 6 years ago by
- Branch changed from u/ncohen/17191 to cc8fd43ab6d177faa74fd90b9979d5ff17da4bb7
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #17191: Poset: change rank dict to rank array