Skip to content

Investigations #108

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.

Sign up for GitHub

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

Jump to bottom
Merged
willrogers merged 5 commits into from
Jul 10, 2019
Merged

Investigations #108

willrogers merged 5 commits into from
Jul 10, 2019

Conversation

@willrogers
Copy link
Collaborator

@willrogers willrogers commented Jul 9, 2019

@aawdls @T-Nicholls further refinements from testing.

willrogers added 4 commits July 9, 2019 16:25
@willrogers
Ensure each element has its own unitconv object.
2566e05 
Although they may have been loaded in the same way, the different
elements might have different limits on their power supplies so we need
different unitconv objects to store that information.
@willrogers
Load unitconv name properly.
76b2fa0
@willrogers
Restore more informative error message for unitconv failure.
e3b80de
@willrogers
A NullUnitConv object doesn't need a name.
3af0f5a
@aawdls
Copy link
Contributor

aawdls commented Jul 9, 2019

Virtual accelerator now starts for me fine without the unit conversion error.

@coveralls
Copy link

coveralls commented Jul 9, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.001%) to 99.474% when pulling 0233d70 on investigations into 196b44e on master.

aawdls
aawdls reviewed Jul 9, 2019
View reviewed changes
@willrogers willrogers merged commit 5e65938 into master Jul 10, 2019
@willrogers willrogers deleted the investigations branch July 10, 2019 10:17
@T-Nicholls
Copy link
Collaborator

T-Nicholls commented Jul 10, 2019

A note, NullUnitConv objects can have conversion limits, though are unlikely to, and so it is possible that a name might be useful for differentiating between unitconv's in this case.
This is entirely theoretical as load_csv does not check for conversion limits for NullUnitConvs, and I don't think it should until a practical use case arrives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@aawdls aawdls aawdls left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@willrogers @aawdls @coveralls @T-Nicholls