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

Fix/issue 609 #616

Merged
merged 5 commits into from
Dec 3, 2021
Merged

Fix/issue 609 #616

merged 5 commits into from
Dec 3, 2021

Conversation

MaxJPRey
Copy link
Collaborator

@MaxJPRey MaxJPRey commented Dec 2, 2021

Fix the issue #609 .
The variable decomposition was not working for units including both letters and digits such as 'm2'.

With the new modification we get the index of the first letter in the variable string and then we split it to get the unit and the value.

units = _find_units_in_dependent_variables(variable_value, full_variables)

if loc:
loc_units = loc.span()[0]
extract_units = variable_value[loc_units:]
if unit_system(extract_units):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maxcapodi78 This unit_system() method is using the dictionary AEDT_UNITS. This dictionary does not contain units such as area m², volume m^3. Consequently it was not working for "3.123456m2" as described in issue #609.

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it. Do you think we should add it or your fix handles all conditions?

@MaxJPRey MaxJPRey requested a review from maxcapodi78 December 2, 2021 08:54
Copy link
Collaborator

@maxcapodi78 maxcapodi78 left a comment

Choose a reason for hiding this comment

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

Great job. Thanks also for detailed UT

@MaxJPRey MaxJPRey merged commit 290016c into main Dec 3, 2021
@MaxJPRey MaxJPRey deleted the fix/issue_609 branch December 3, 2021 14:12
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.

2 participants