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

LatticePoset: join and meet of several elements #17231

Closed
jm58660 mannequin opened this issue Oct 27, 2014 · 12 comments
Closed

LatticePoset: join and meet of several elements #17231

jm58660 mannequin opened this issue Oct 27, 2014 · 12 comments

Comments

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Oct 27, 2014

Enhance join() and meet() to accept a list as an argument. If empty list is allowed, return the top/bottom element.

CC: @nathanncohen

Component: combinatorics

Author: Jori Mäntysalo

Branch/Commit: 51aa0ae

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/17231

@jm58660 jm58660 mannequin added c: combinatorics labels Oct 27, 2014
@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 27, 2014

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 27, 2014

Commit: 8095cf3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 27, 2014

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

8095cf3Enhanced meet() and join() to accept a list of elements as argument.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 27, 2014

Author: Jori Mäntysalo

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 27, 2014

comment:3

I also removed old example from three years old bug.

@jm58660 jm58660 mannequin added the s: needs review label Oct 27, 2014
@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Oct 27, 2014

comment:4

Hello !

I do not understand why you do

i, j = map(self._element_to_vertex, (x,y))
return self._vertex_to_element(self._hasse_diagram._meet[i,j])

and not only

x=(x,y)

Short of that, you updated the docstring of "join" but did not do the same for "meet".

AAAannd I think that it is all :-)

Nathann

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Oct 28, 2014

comment:5

Replying to @nathanncohen:

I do not understand why you do

i, j = map(self._element_to_vertex, (x,y))
return self._vertex_to_element(self._hasse_diagram._meet[i,j])

and not only

x=(x,y)

I do not understand what you don't understand. :=)

I the function get an element of the lattice as an argument, say 'a', it must convert it to "element" of the Hasse diagram, get meet and convert back. Maybe instead of map it would be better to have just

i = self._element_to_vertex(x)
j = self._element_to_vertex(y)

Or do you mean to remove part I marked as "Handle basic case fast" to comments?

Short of that, you updated the docstring of "join" but did not do the same for "meet".

True. Must correct this.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2014

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

51aa0aeDocstring of meet() corrected.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2014

Changed commit from 8095cf3 to 51aa0ae

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Oct 28, 2014

Reviewer: Nathann Cohen

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Oct 28, 2014

comment:8

Hello !

Well, I thought that you could save some time but now I do not see it anymore O_o

It feels a bit weird to compute the top element, and compute its meet with something else given that you already know the result but well, it does not matter much I guess.

Good to go !

Nathann

@tscrim tscrim added this to the sage-6.4 milestone Oct 29, 2014
@vbraun
Copy link
Member

vbraun commented Nov 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants