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

Change from unit="1" to unit="" to avoid unit issues. #3879

Closed
wants to merge 1 commit into from

Conversation

HansOlsson
Copy link
Contributor

Closes #3878

@mestinso
Copy link
Collaborator

Instead of making the change here, why not just change the default units for gain in the various MSL Blocks (Math.Gain, Continuous.Integrator, etc.)? It seems to me the choice of setting the units for gain = "1" wasn't a good general assumption in those components and they should just be set to "". I can see the logic in the context of a "voltage gain" or something along those lines, but I don't think it's generally correct assumption in other contexts (controls etc.) like in the examples here. Thoughts?

@HansOlsson
Copy link
Contributor Author

Instead of making the change here, why not just change the default units for gain in the various MSL Blocks (Math.Gain, Continuous.Integrator, etc.)? It seems to me the choice of setting the units for gain = "1" wasn't a good general assumption in those components and they should just be set to "".

That was also my first reaction, which basically means reverting #227 and I'm open to it.

However, when I looked the number of problematic cases were actually fairly low (as listed in this PR).

But it could be that the cases helped by #227 are also so few that it isn't worth the effort (I see that there are more gain-blocks - but I don't see if unit-checking would do anything for the other cases).

We could even decide to mix&match to e.g. remove unit for Gain, and keep for the others such as Integrator (and thus modify even fewer cases).

Copy link
Member

@dietmarw dietmarw left a comment

Choose a reason for hiding this comment

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

I would prefer to use the solution presented in #3881 instead.

@HansOlsson
Copy link
Contributor Author

Since #3881 seems more popular I will close this one. Obviously it can re-opened if that changes.

@HansOlsson HansOlsson closed this Oct 27, 2021
@beutlich beutlich added this to the never milestone Oct 27, 2021
@beutlich beutlich added the invalid Invalid issue label Oct 27, 2021
@HansOlsson HansOlsson deleted the CorrectGainUnit branch June 15, 2023 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Invalid issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit-checking using Gain and LimIntegrator-blocks
4 participants