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

instance.get_pins() vs instance.pins #139

Open
jacobdbrown4 opened this issue May 24, 2021 · 6 comments
Open

instance.get_pins() vs instance.pins #139

jacobdbrown4 opened this issue May 24, 2021 · 6 comments

Comments

@jacobdbrown4
Copy link
Collaborator

This is probably a minor issue, but right now, instance.get_pins() returns inner pins but instance.pins returns outer pins. Shouldn't they both return outer pins?

@jacobdbrown4 jacobdbrown4 changed the title instances.get_pins() vs instances.pins instance.get_pins() vs instance.pins May 24, 2021
@thunder-hammer
Copy link
Collaborator

good catch. I believe that the get_pins() call has the option to return either inner or outer pins and defaults to inner. i'll do a little bit of digging in the code and let you know what I find.

@thunder-hammer
Copy link
Collaborator

thunder-hammer commented May 25, 2021

Could you try something like the following to see what happens?

from spydrnet.util.selection import Selection

and then run something like

get_pins(selection = Selection.OUTSIDE)

Let me know if that gets the outer pins

I think we made selection = Selection.INSIDE the default on everything. The instance's pins case might be one where it doesn't make as much sense. I think we were thinking if you wanted to get all cables attached to an instance or definition, you might be interested in the ones that are inside of it or outside. We just extended that to the pins.

As far as changing this i'm not sure if anyone is relying on the current behavior... We can at the very least check against our internal redundancy applications. We may want to push this off to a version 2 feature.

@jacobdbrown4
Copy link
Collaborator Author

Using the Selection.OUTSIDE does get the outer pins. That makes sense now why you did that...so you could get either the inner or outer pins. Now that I understand it, it doesn't seem like a bug really so we probably don't need to change it.

@benglines benglines reopened this Jul 20, 2021
@benglines benglines added the major Major release issue label Jul 20, 2021
@agg23
Copy link
Contributor

agg23 commented Apr 1, 2023

I'm new to this codebase, but I don't view this as a minor issue. The existing IR construction is already somewhat opaque given the similar names and concepts (InnerPin, OuterPin, Port all represent similar, but different concepts, but have little differentiation in the names), and this just adds further confusion.

I spent several hours scouring the docs and playing with the code, trying to figure out why I couldn't find the wires for a given instance, only to find out that on many of the types, get_* does not return the same same data as a getter for the local property.

  • If Instance.get_pins() is not remotely equivalent to Instance.pins, why are they named nearly the same? As a user, unless instructed otherwise, I would expect either of those calls to give me all relevant pins for that instance. That means the InnerPins from the original Definition, and the OuterPins being used for that particular Instance. Except neither of those actually do that.
  • Why is a default chosen for get_pins() that filters data (you should basically never do this unless there are performance concerns)?
    • If a method filters data by default, why is that not very explicitly called out in the documentation?
    • Why does get_pins() not have a required filter argument? I imagine you probably wanted to avoid a breaking API signature change, but I would expect that filtering by default was a change anyway.
  • How do the get* methods differ from the local getters, what is their intended use, and where is that documented?

On a similar, but unrelated note, why does the Selection enum declare both BOTH and ALL? There appears to be some differentiation, given their uses in the codebase, but that is unclear from the naming.

@AEW2015 AEW2015 reopened this Apr 2, 2023
@jgoeders
Copy link
Member

I agree with @agg23. I wasted a good amount of time on .pins vs .get_pins(), trying to figure out why .get_pins on my Instance was not giving me the OuterPins.

I don't think it makes sense to have getter and a property that sound to do the same thing. It sounds like get_pins() should be renamed to something else...

@jacobdbrown4
Copy link
Collaborator Author

Thanks for the feedback. This should definitely be improved. I'll take a look at it soon.

@jacobdbrown4 jacobdbrown4 added this to Summer Work in Fall 2023 Release Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Major release issue
Projects
Fall 2023 Release
Address Soon
Development

No branches or pull requests

6 participants