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

Update wrapAngle.mo #3975

Merged
merged 3 commits into from
Jun 30, 2022
Merged

Update wrapAngle.mo #3975

merged 3 commits into from
Jun 30, 2022

Conversation

lvanfretti
Copy link
Member

Bracket typos in function description and documentation.

Bracket typos in function description and documentation.
@CLAassistant
Copy link

CLAassistant commented Apr 18, 2022

CLA assistant check
All committers have signed the CLA.

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 think this PR is invalid since the documentation is correct it with respect that the range does not include the lower -pi or the upper 2*pi. Hence ]-pi,pi] and [0,2*pi[are correct.

@dietmarw
Copy link
Member

One could argue though that the non-standard ]-pi,pi] and [0,2*pi[ should be changed to standard notation (-pi,pi] and [0,2*pi) but I'm not sure that this is really that important also since it means changing it in several other places in the MSL.

@dietmarw dietmarw added the L: Blocks Issue addresses Modelica.Blocks label Apr 19, 2022
@HansOlsson
Copy link
Contributor

One could argue though that the non-standard ]-pi,pi] and [0,2*pi[ should be changed to standard notation (-pi,pi] and [0,2*pi) but I'm not sure that this is really that important also since it means changing it in several other places in the MSL.

Both variants are supposed to be standard notation.
https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints
https://en.wikipedia.org/wiki/ISO_31-11

@dietmarw
Copy link
Member

dietmarw commented Apr 19, 2022

Ah ok, in that case Wolfram ought to update its pages ;-) https://mathworld.wolfram.com/OpenInterval.html

Anyway, I would close this PR as invalid but leave the decision to the library officers @AHaumer and @MartinOtter

@lvanfretti
Copy link
Member Author

Both variants are supposed to be standard notation. https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints https://en.wikipedia.org/wiki/ISO_31-11

As a plain vanilla user, the notation (-pi,pi] and [0,2*pi) is easier to understand. Most people are familiar with that one... that is what it confused me in the first place.

@lvanfretti
Copy link
Member Author

Yes, I understand. As I said, it is just a less common notation used in the documentation that what one would expect a normal user to know. Perhaps, adding a clarification could be better than modifying the existing text?

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.

[-pi,pi] is wrong.
I'm comfortable both with ]-pi,pi] and (-pi,pi].
So use on of the two formulations.

@christiankral
Copy link
Contributor

OK, depending on this discussion I propose:

  1. to keep the original implementation
  2. add a link to https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints so that pepole not familiar with the used nomenclature can look it up

I am adding the link to the block and the function.

Add link to Wikipedia
@dietmarw dietmarw added this to the MSL4.1.0 milestone Jun 14, 2022
@dietmarw dietmarw added the documentation Issue addresses the documentation label Jun 14, 2022
@AHaumer AHaumer merged commit a497604 into modelica:master Jun 30, 2022
@beutlich beutlich removed the request for review from MartinOtter July 1, 2022 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue addresses the documentation L: Blocks Issue addresses Modelica.Blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants