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

Add automaticLocalSeed in Math.Random and utilize in noise blocks #3224

Merged
merged 5 commits into from
Nov 13, 2019

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Nov 11, 2019

There was some in and out development going on in #1957 when the noise blocks and functions were introduced in MSL v3.2.2 with the automaticLocalSeed left to do. This pull request fixes #1861.

@akloeckner @dzimmer @fvanderlinden FYI

@beutlich beutlich added enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks L: Math Issue addresses Modelica.Math V: 3.2.2 Issue originates in MSL v3.2.2 (and is not present in earlier releases) labels Nov 11, 2019
@beutlich beutlich self-assigned this Nov 11, 2019
@beutlich beutlich requested review from tobolar and removed request for bernhard-thiele November 11, 2019 20:29
@HansOlsson
Copy link
Contributor

Two comments:

  • Having a predictable sequence based on instance name is good, but as I understand we currently have the possibility to generate different results when re-running the same model - and I believe we should keep that possibility.
  • Why is the hashing function spending time to split off the component-path? Unless the hash-function is really broken a fixed prefix doesn't cause any problems. If the goal is to generate the same result regardless of simulation model name I believe that will fail for some quoted identifiers.

@beutlich
Copy link
Member Author

beutlich commented Nov 12, 2019

Having a predictable sequence based on instance name is good, but as I understand we currently have the possibility to generate different results when re-running the same model - and I believe we should keep that possibility.

I guess it is the other way round as it was an unintended side-effect that the automatically generated local seed somehow depended on the number/order of noise blocks in the model. This is now circumvented in such a way that this automatically generated local seed only depends on the component name, no matter how many noise blocks are found in the model

Why is the hashing function spending time to split off the component-path? Unless the hash-function is really broken a fixed prefix doesn't cause any problems. If the goal is to generate the same result regardless of simulation model name I believe that will fail for some quoted identifiers.

True. See also #1957 (comment). I'll fix automaticLocalSeed to use the plain path input as hashString input.

@HansOlsson
Copy link
Contributor

I guess it is the other way round as it was an unintended side-effect that the autmatically generated local seed somehow depended on the number/order of noise blocks in the model. This is now circumvented in such a way that this autmatically generated local seed only depends on the component name, no matter how many noise blocks are found in the model

Ah, yes, I see - we have kept that possibility; just more predictably.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Looks good now.

@beutlich beutlich removed the request for review from tobolar November 13, 2019 10:41
@beutlich beutlich merged commit 2c06613 into modelica:master Nov 13, 2019
@beutlich beutlich deleted the update-automaticLocalSeed branch November 13, 2019 10:41
@beutlich beutlich added this to the MSL4.0.0 milestone Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks L: Math Issue addresses Modelica.Math V: 3.2.2 Issue originates in MSL v3.2.2 (and is not present in earlier releases)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Noise: Improve automatic local seed function
3 participants