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:

Status badges

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 ncohen

  • Authors set to Nathann Cohen
  • Branch set to u/ncohen/17191
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by git

  • Commit set to 08727b253f8ecc25285f2e0c01a21424868bcf63

Branch pushed to git repo; I updated commit sha1. New commits:

​08727b2trac #17191: Poset: change rank dict to rank array

comment:3 Changed 6 years ago by jmantysalo

  • 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: Changed 6 years ago by ncohen

Yooo ! 

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__?

You are right. 

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.

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 ncohen

  • Status changed from needs_work to needs_review

comment:6 Changed 6 years ago by git

  • Commit changed from 08727b253f8ecc25285f2e0c01a21424868bcf63 to 45fc474b60ea8de4d76f4aa98c64b9b2765743fe

Branch pushed to git repo; I updated commit sha1. New commits:

​45fc474trac #17191: Reviewer's comments

comment:7 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by jmantysalo

  • 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 or return 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 ncohen

Yo !

Ah, true. This could also start with rank_fcn=[None]*self.order() instead of rank_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 git

  • 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:

​cc8fd43trac #17191: Doing the job properly

comment:10 Changed 6 years ago by jmantysalo

  • Status changed from needs_review to positive_review

Works.

comment:11 Changed 6 years ago by ncohen

Thanks !

Nathann

comment:12 Changed 6 years ago by vbraun

  • Branch changed from u/ncohen/17191 to cc8fd43ab6d177faa74fd90b9979d5ff17da4bb7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.