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

Feature: bidirectional AWG/mm2 unit conversion #41

Merged
merged 7 commits into from
Jun 29, 2020

Conversation

n42
Copy link
Contributor

@n42 n42 commented Jun 29, 2020

Broke out this feature from PR #9 and resubmitting separately with minor changes after rebase:

  • Feature: Add automatic AWG->mm2 conversion as well, when using show_equiv. Some minor refactoring there as well, modified ex02 to show it the results. Note that the lookup table has been changed to strings on both sides, this is because e.g. '000', '2/0' and the like are perfectly valid AWG values.

  • Fixes [Feature] Bidirectional awg<->mm2 unit conversion #40

formatc1702 and others added 5 commits June 28, 2020 15:00
Add an inverted dictionary and a lookup function from awg -> mm2. Also
do some minor refactoring. Both sides of the conversion table were
converted to strings, since '0000' and '2/0' are perfectly valid AWG
values.
Show conversions for ex02, and make sure it displays conversions in both
directions. Rebuild the example files.
@formatc1702
Copy link
Collaborator

This looks great. Can we have AWG be output in upper case, and have it detect upper and lower case in the YAML input? It's the only nit-pick I can come up with ;)

@n42
Copy link
Contributor Author

n42 commented Jun 29, 2020

Hmm the converted value is (hardcoded) upper case, the lowercase you see is from the original YAML; that's passed directly into the gauge_unit variable and used as is. To clarify, do you mean that 1) all AWG strings should be converted to upper case for rendering, or that 2), all AWG strings should respect and use the casing from the YAML file? The first i think should be separately refactored, but it would yield more consistent output. The second would probably be a bit of a pain, because the converted values won't have a YAML original AWG string to base casing on, and if we inspect the rest of the YAML input we might very well find multiple different cases. In fact, the input isn't actually sanity checked, so it's quite possible to specify anything other than mm2 as e.g. 'MuricanWireGauge' or 'bananas' in the YAML, currently.

@formatc1702
Copy link
Collaborator

Your edit beat me to the punch.

I think AWG should always be rendered as upper case, but we do need to take into account where unknown units ('bananas', square inches, ...) are used, these should simply be passed through, with no equivalent conversion and no case conversion either.

@n42
Copy link
Contributor Author

n42 commented Jun 29, 2020

Okay, so for the purposes of this PR:

  • retain mm2 -> awg conversion, label as 'AWG'
  • add check for gauge_type == awg before othe -> mm2 conversion
  • leave anything else as is even if show_equiv == true
  • separate refactor PR for upper-i-fying all AWG string occurances

sound about right?

The parsing allows arbitrary units to be used for cable dimensions --
this might be valid units, e.g. square inches, or invalid, e.g. bananas.
We only allow conversion between mm2 and AWG, so check that the
gauge_unit is either of those before conversion. If not, pass through as
is.
@formatc1702
Copy link
Collaborator

Sounds good. If there's a separate PR coming for the last point, is this one ready for merging?

@n42
Copy link
Contributor Author

n42 commented Jun 29, 2020

Actually that was a quick fix, pushing a commit with that as well in a sec seeing as it's related

Convert e.g. 'awg, 'AwG' to upper case for consistent rendering. Leave
any other input gauge units as they were.
if cable.show_equiv:
if cable.gauge_unit =='mm\u00B2':
awg_fmt = f' ({awg_equiv(cable.gauge)} AWG)'
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be elif cable.gauge_unit.upper() == 'AWG' or cable.gauge_unit == '' or something similar to avoid converting 'bananas' to AWG by assuming 'bananas' equals mm2?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think that is a good call

Copy link
Collaborator

Choose a reason for hiding this comment

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

The 'AWG' and 'mm' constant strings should also be defined only once in the helper file and then use identifiers in the rest of the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 especially the Unicode escaping is a pain. Will you include the fix in this PR? should be easy enough

Copy link
Contributor Author

@n42 n42 Jun 29, 2020

Choose a reason for hiding this comment

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

yeah, that was fixed in 625b21e. And i agree about the multiple definitions, but that's a separate refactor job IMO. Plus I need to go do some actual paid work for a while, so it won't get done for a couple of hours, and I'd rather get this closed out in the meanwhile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you. Let me know when you're ready to merge this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're fine with the string casing fix for now i'd say go for merge

Copy link
Collaborator

Choose a reason for hiding this comment

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

@n42 I'm sorry for pointing out the issue you already had fixed, but I could not see your fix at that time. Github have been slow and somewhat unstable today, so I guess my view was not fully updated.

@n42
Copy link
Contributor Author

n42 commented Jun 29, 2020

Figured that might as well be done early-on, so put it alongside the mm2 -> mm^2 conversion in the dataclass

@formatc1702 formatc1702 merged commit 08e53bc into wireviz:dev Jun 29, 2020
formatc1702 added a commit that referenced this pull request Jun 29, 2020
formatc1702 added a commit that referenced this pull request Jun 29, 2020
@n42 n42 deleted the feature-unit_conversion branch June 29, 2020 13:08
@formatc1702 formatc1702 added this to the v0.2 milestone Jul 19, 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.

3 participants