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

Reference wires by color or label #194

Merged
merged 3 commits into from
Nov 14, 2020
Merged

Reference wires by color or label #194

merged 3 commits into from
Nov 14, 2020

Conversation

formatc1702
Copy link
Collaborator

@formatc1702 formatc1702 commented Nov 11, 2020

Closes #70.
Closes #169.
Closes #193.

@formatc1702
Copy link
Collaborator Author

I'm sure the Harness.connect() function can be simplified and cleaned up quite a bit. My hope is to include this feature soon (we have been needing this at work and I finally found some time for it) and do some cleanup in the upcoming major refactoring.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 11, 2020

Update: Wire number can now be displayed as suggested in #169. It is controlled by the Cable.show_wirenumbers parameter, which defaults to True for "normal" cables (which usually follow a numbering scheme) and to False for bundles (which have no particular order), but can be easily overridden.

Expand to see examples 2 (cables, numbered wires) and 3 (bundle, no numbered wires), as well as a modified example 8 (numbered wires + labels)

ex02

ex03

ex08

If there are cables with more exotic/complex wire numbering schemes, those will not be handled right now.

@kvid
Copy link
Collaborator

kvid commented Nov 11, 2020

Update: Wire number can now be displayed as suggested in #169. It is controlled by the Cable.show_wirenumbers parameter, which defaults to True for "normal" cables (which usually follow a numbering scheme) and to False for bundles (which have no particular order), but can be easily overridden.

I suggest also default True when neither wire colors nor wire labels can identify all wires unambiguously, because then wire numbers are the only unambiguous wire identifier.

Expand to see examples 2 (cables, numbered wires) and 3 (bundle, no numbered wires), as well as a modified example 8 (numbered wires + labels)

I suggest showing any empty color or label as an empty string to avoid skipping a ':' separator in such cases, because if some wires are shown with fewer separators, then it is unambiguous which of the identifiers are empty.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 11, 2020

Update: Adressed #70 as well.

image

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 11, 2020

Update: Wire number can now be displayed as suggested in #169. It is controlled by the Cable.show_wirenumbers parameter, which defaults to True for "normal" cables (which usually follow a numbering scheme) and to False for bundles (which have no particular order), but can be easily overridden.

I suggest also default True when neither wire colors nor wire labels can identify all wires unambiguously, because then wire numbers are the only unambiguous wire identifier.

This would only be the case for bundles with ambiguous colors/labels... in which case, the concept of having a numbering no longer applies IMHO. In that case, both in the diagram and in real life, following a wire from one end to the other is the only way to make sure, and I don't see a need for complicating the program logic here.
If it's a (non-bundle) cable with ambiguous colors/labels, it is already defaulting to True.

Expand to see examples 2 (cables, numbered wires) and 3 (bundle, no numbered wires), as well as a modified example 8 (numbered wires + labels)

I suggest showing any empty color or label as an empty string to avoid skipping a ':' separator in such cases, because if some wires are shown with fewer separators, then it is unambiguous which of the identifiers are empty.

Can you provide a realistic example of where this would be an issue? If a cable has defined colors, surely there would be no gap in the color list? As for labels, if some wires have none, the final :<label> goes missing, but the position of the wire number (if shown) and wire color stay consistent.

If a cable has wire labels but no set colors, it's more readable to simply leave out the color section altogether, instead of filling it with -- (or similar) or having a double : (e.g. 1::label) for no real reason...

src/wireviz/Harness.py Outdated Show resolved Hide resolved
@kvid
Copy link
Collaborator

kvid commented Nov 12, 2020

I suggest also default True when neither wire colors nor wire labels can identify all wires unambiguously, because then wire numbers are the only unambiguous wire identifier.

This would only be the case for bundles with ambiguous colors/labels... in which case, the concept of having a numbering no longer applies IMHO. In that case, both in the diagram and in real life, following a wire from one end to the other is the only way to make sure,

Well, the numbering still applies as the rendering order in the diagram and for specifying connections - the only way in this case. And in real life there are cases where the wires have an order, either some kind of numeric labels or e.g. the order they are strapped to the surrounding structure.
ordered unicolored wires

However, the user can always specify Cable.show_wirenumbers: True manually in such cases.

and I don't see a need for complicating the program logic here.

That might be a valid argument. The program logic complexity would be at least twice the similar test for unique connector pins, i.e. something like this:

all(len(ids) < self.wirecount or len(ids) != len(set(ids)) for ids in [self.colors, self.wirelabels])

If it's a (non-bundle) cable with ambiguous colors/labels, it is already defaulting to True.

That's true.

Expand to see examples 2 (cables, numbered wires) and 3 (bundle, no numbered wires), as well as a modified example 8 (numbered wires + labels)

I suggest showing any empty color or label as an empty string to avoid skipping a ':' separator in such cases, because if some wires are shown with fewer separators, then it is unambiguous which of the identifiers are empty.

Can you provide a realistic example of where this would be an issue? If a cable has defined colors, surely there would be no gap in the color list?

You are probably right. After reading your current code changes, I'm not that worried. 😃

As for labels, if some wires have none, the final :<label> goes missing, but the position of the wire number (if shown) and wire color stay consistent.

When some labels are shown, and some are empty, I would prefer a trailing ':' for the wires with an empty label to easily see where labels are missing, but I accept that you disagree.

If a cable has wire labels but no set colors, it's more readable to simply leave out the color section altogether, instead of filling it with -- (or similar) or having a double : (e.g. 1::label) for no real reason...

I agree with you about the cases where the whole list (cable.colors or cable.wirelabels) is empty.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 12, 2020

I suggest also default True when neither wire colors nor wire labels can identify all wires unambiguously, [...]

[...] And in real life there are cases where the wires have an order, either some kind of numeric labels [...]

<troll> I'm afraid you've gone full circle here 😛 </troll> (SCNR)

or e.g. the order they are strapped to the surrounding structure.

That one is a valid point. Let's leave the logic as-is for now and let the user override in this situation.

When some labels are shown, and some are empty, I would prefer a trailing ':' for the wires with an empty label to easily see where labels are missing, but I accept that you disagree.

I'll think about it; it's not completely unreasonable, but I'm leaning towards leaving the trailing : out.

@kvid
Copy link
Collaborator

kvid commented Nov 12, 2020

I suggest also default True when neither wire colors nor wire labels can identify all wires unambiguously, [...]

[...] And in real life there are cases where the wires have an order, either some kind of numeric labels [...]

<troll> I'm afraid you've gone full circle here 😛 </troll> (SCNR)

LOL 🤣 I see your point. 😆 I was thinking of a case where the diagram wire labels (and possible physical written labels) represent the wire function, and that it could exist some kind of numeric marks on the wires as well, but a better way to handle that would be to prefix the diagram wire labels with the numeric marks to obtain unambiguous labels.

or e.g. the order they are strapped to the surrounding structure.

That one is a valid point. Let's leave the logic as-is for now and let the user override in this situation.

I'm perfectly fine with that.

When some labels are shown, and some are empty, I would prefer a trailing ':' for the wires with an empty label to easily see where labels are missing, but I accept that you disagree.

I'll think about it; it's not completely unreasonable, but I'm leaning towards leaving the trailing : out.

I can live with either solution. It's not a major issue to me, I was only trying to explain the rationale behind my original suggestion in a better way.

@formatc1702
Copy link
Collaborator Author

I've rebased onto the latest dev after merging #141; hopefully this one can follow soon after :)

@kvid
Copy link
Collaborator

kvid commented Nov 14, 2020

I've rebased onto the latest dev after merging #141;

👍

hopefully this one can follow soon after :)

Do you wait for an OK from me? As your previous comment included "I'll think about it...", then I thought I would wait until you concluded before reviewing again. Have you made any recent changes (other than rebasing)? I don't have my PC here to inspect diffs.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 14, 2020

Sorry for being vague. I prefer leaving the trailing : out; we can add it back in if the arguments in favor increase. I haven't changed anything besides the rebase.
So please have a look whether you find any major problems... like I said, once we go object-oriented, a lot of the code will hopefully be simplified, so I wouldn't look to optimize it to perfection this time ;-) Thanks!

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being vague.

No worries. I'm sorry I misunderstood you. As you requested a quick feedback without perfection in mind, I have mainly rebuilt all examples and the diff seems as expected. Thank you also for accepting my earlier suggestion. I don't have the time to look for simplifications of the connection code right now, but I guess that can be done later. Sorry for not being as thorough as usual, but I understand that is what you want this time.

@formatc1702 formatc1702 merged commit eebf932 into dev Nov 14, 2020
@formatc1702 formatc1702 deleted the feature/wirelabels branch November 14, 2020 22:21
@formatc1702
Copy link
Collaborator Author

Awesome, thanks!
I'll post a quick summary of my near future plans in #101.

formatc1702 added a commit that referenced this pull request Nov 14, 2020
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.

[feature] better readability for cables with numbered wires
2 participants