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

Add new get elements method to lattice to return elements by family or class. #102

Closed
wants to merge 3 commits into from

Conversation

T-Nicholls
Copy link
Contributor

A first draft of my proposed solution to #100.
Perhaps having two separate get_elements_by_family and get_elements_by_class would be more elegant than my current implementation; what is everyone's opinion on this?
I would welcome any feedback or improvements that anyone might have.

@willrogers
Copy link
Contributor

cc @lfarv @abdomit

@abdomit
Copy link
Contributor

abdomit commented Mar 30, 2019

Thanks @T-Nicholls for addressing #100!

I prefer simple names, so having just one get_elements function is fine. However I am not so comfortable with the idea that a str could be either a class name or a FamName. What if someone calls an element 'ap' (a 2 letter-acronym is easy to find), do a get_element('ap') and get all Aperture elements instead? Maybe one could say that giving a str to get_elements will just address FamName?

Also throwing an exception if there is no element matching the str seems a bit excessive. Returning an empty list is also fine, don't you think?

@lfarv
Copy link
Contributor

lfarv commented Mar 30, 2019

I agree with @abdomit on the "string" option for the key: it should refer to FamName only. The "Class" name is a pure Matlab feature and does not appear any more in the Python object. We should forget it.

I also think that we should get an empty list in case no element matches. For me it is not an error.

Otherwise:

  • Shouldn't this function belong to lattice.utils and be available more generally to any sequence of elements, with a corresponding Lattice method, like it is done for get_s_pos?
  • Checking versus the str type ignores the Python 2 unicode type. But Python 2 is at end of life, so this looks good enough for me.

Final note: this function ends up with either
elems = [elem for elem in self if fnmatch(elem.FamName, key)]
or
elems = [elem for elem in self if isinstance(elem, type(key))]
or
elems = [elem for elem in self if isinstance(elem, key)]

For me, using any of these single lines looks as simple and clear as a function call. But I have nothing against the function!

@T-Nicholls
Copy link
Contributor Author

Thank you both for the feedback, I agree with everything said so I'll make the changes.
However, I will use numpy.issubdtype for now since we might as well support unicode strings.

@T-Nicholls
Copy link
Contributor Author

@lfarv @abdomit
When you are both happy with this I would like to put close this pull request and move these changes onto the end on #103 as the test is failing because it relies on a fixture from that branch and we would have a conflict on merging the second one of these branches anyway, so I think it is easier for me to move it manually.

@abdomit
Copy link
Contributor

abdomit commented Apr 1, 2019

@T-Nicholls: I would only suggest that you add an optional verbose mode to prevent being flooded by messages when calling get_elements with a str as parameters.

Otherwise it looks nice to me. Thanks again.

@T-Nicholls
Copy link
Contributor Author

No problem, I will add a quiet argument, True by default to stop the print statement.

T-Nicholls added a commit to T-Nicholls/at that referenced this pull request Apr 1, 2019
@T-Nicholls T-Nicholls closed this Apr 1, 2019
@lfarv
Copy link
Contributor

lfarv commented Apr 2, 2019

Hi @T-Nicholls. What happened to this branch? It looked ready for merging, but I do not see it any more…

@T-Nicholls
Copy link
Contributor Author

T-Nicholls commented Apr 2, 2019

@lfarv
As mentioned above I moved these changes onto #103 because the test I wrote for get_elements relied on changes made on that branch. There were also conflicts between the two, so I figured I might as well sort it out on my end and put it all on one pull request since as, for me, #103 is nearly ready to merge (I just have one more commit's worth of changes). Sorry for any confusion.

@T-Nicholls T-Nicholls deleted the getElements branch April 2, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants