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:  sage6.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 followup: ↓ 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 ; followup: ↓ 8 Changed 6 years ago by
 Milestone changed from sagewishlist to sage6.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