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 unit error in Thyristor #4119

Merged
merged 3 commits into from Jan 14, 2024
Merged

Conversation

qlambert-pro
Copy link
Contributor

Introduces a variable to resolve a unit error in an equation.

We should give a better name to the variable I have introduced.

Introduces a variable to resolve a unit error in an equation.
Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

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

I'm not the author of this model, and I don't know the meaning of "vRef = 0.65".
It might be better to search for a name explaining the nature of it, and to declare it either as (maybe protected) constant or as a parameter.
@christophclauss can you tell us more about this "secret value"?
BTW, I can't figure out why a tool should issue a unit error?
Isn't it possible to deduce that "0.65" is a constant whose quantity is voltage?
@HansOlsson or othe friends from Map-Lang could you please comment on this?

@HansOlsson
Copy link
Contributor

BTW, I can't figure out why a tool should issue a unit error? Isn't it possible to deduce that "0.65" is a constant whose quantity is voltage? @HansOlsson or othe friends from Map-Lang could you please comment on this?

There are two parts to that answer:

  • The problematic part is 0.65^2/VGT or vRef^2/VGT; the other uses of 0.65 should be allowed by the 4-line proposal. The a=0.65*b; are exactly the kind of equations that the proposal aims to use unit-checking to help detect errors in.
  • If there were a constant with value 0.65 we could in fact deduce its unit, but we obviously cannot assume that all 0.65 have the same unit; but having a "Real vRef=0.65;" and deducing its unit seems weird.

@henrikt-ma
Copy link
Contributor

  • If there were a constant with value 0.65 we could in fact deduce its unit, but we obviously cannot assume that all 0.65 have the same unit; but having a "Real vRef=0.65;" and deducing its unit seems weird.

I'll just expand slightly on this. In the unit handling discussions at the last physical language meeting, there was some sort of vague conclusion that we should probably deprecate declaring constants without explicit unit. This would make it easier to reason about a the constants in Modelica libraries, as they would always have the same unit regardless of application. Also, since a package constant doesn't need to belong to an instance, we avoid the problem of having inferred a unit for something which isn't present in the translated model (and hence is difficult to communicate to the user).

Copy link
Member

@hubertus65 hubertus65 left a comment

Choose a reason for hiding this comment

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

By decision on 2023-11-14

@hubertus65
Copy link
Member

I'm not the author of this model, and I don't know the meaning of "vRef = 0.65". It might be better to search for a name explaining the nature of it, and to declare it either as (maybe protected) constant or as a parameter. @christophclauss can you tell us more about this "secret value"? BTW, I can't figure out why a tool should issue a unit error? Isn't it possible to deduce that "0.65" is a constant whose quantity is voltage? @HansOlsson or othe friends from Map-Lang could you please comment on this?

I think your questions have been addressed, please re-review and approve, @AHaumer

@HansOlsson
Copy link
Contributor

HansOlsson commented Nov 15, 2023

I haven't reviewed this - so I will just comment more:

As far I understand 0.65V is a common "Forward threshold" or knee-voltage for Silicon diodes (well 0.6V to 0.7V); I assume Thyristor have some electrical similarities to diodes, but it was a long time since I took those courses.

Thus I don't know an appropriate name if vRef isn't good enough.

BTW: It is plausible that changing just that value makes sense for other materials, but I don't know. That will impact whether it should be a public non-final parameter or not.

Co-authored-by: Hans Olsson <HansOlsson@users.noreply.github.com>
@christiankral
Copy link
Contributor

BTW: It is plausible that changing just that value makes sense for other materials, but I don't know. That will impact whether it should be a public non-final parameter or not.

Even though I am not familiar with the transistor model I would rather go for a final parameter. If there is a need for a public parameter, we can change it at any time in a backwards compatible way.

Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

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

Fine with me

@beutlich beutlich added this to the MSL4.1.0 milestone Jan 14, 2024
@arunkumar-narasimhan arunkumar-narasimhan merged commit 5197646 into modelica:master Jan 14, 2024
2 checks passed
@beutlich beutlich added the L: Electrical.Analog Issue addresses Modelica.Electrical.Analog label Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Electrical.Analog Issue addresses Modelica.Electrical.Analog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants