Sensor random seed by seanmcleod70 · Pull Request #1254 · JSBSim-Team/jsbsim

seanmcleod70 · 2025-04-07T21:14:52Z

No description provided.

codecov · 2025-04-08T11:02:21Z

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 24.78%. Comparing base (2006648) to head (6f30e20).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/models/flight_control/FGSensor.cpp	0.00%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1254      +/-   ##
==========================================
- Coverage   24.94%   24.78%   -0.17%     
==========================================
  Files         169      169              
  Lines       19524    19656     +132     
==========================================
+ Hits         4871     4872       +1     
- Misses      14653    14784     +131

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bcoconni · 2025-04-13T12:48:21Z

For the record, this PR is motivated by #1250 (comment).

@seanmcleod70, I guess we also need a property randomseed for these sensors similar to simulation/randomseed as well as atmosphere/randomseed from #1253 ? At least, one would need that to reset the simulation to its initial state, no ?!?

seanmcleod70 · 2025-04-13T19:49:32Z

The random seed for a particular sensor, i.e. a sensor that you want to control the reproducibility of it's noise is set via a sensor attribute, specific to a specific sensor.

<sensor name="name">
 <input> property </input>
 <lag> number </lag>
 <noise [variation="PERCENT|ABSOLUTE"] [distribution="UNIFORM|GAUSSIAN"] [randomseed="number"] > number </noise>

So unlike the wind turbulence case there isn't a single random seed property.

Not quite following you in terms of "reset the simulation to it's initial state".

In terms of reset are you talking about the case where an FDM is created, it's run for a while, then reset and further time steps, i.e. without creating a new FDM instance?

fdm = create_fdm()
fdm.run()  # N times
fdm.reset()
fdm.run() # N times

bcoconni · 2025-04-14T10:13:24Z

So unlike the wind turbulence case there isn't a single random seed property.

Yep, got that. But my question was rather if a property would not be needed for each sensor that uses this attribute in its XML definition ?

In terms of reset are you talking about the case where an FDM is created, it's run for a while, then reset and further time steps, i.e. without creating a new FDM instance?

Correct, that was my thought. I was trying to anticipate some cases where having a property would be useful but maybe it's irrelevant ?

seanmcleod70 · 2025-04-14T18:25:17Z

I did sort of consider the option of generating a property for the sensor if it had a <noise/> element with a name attribute along the lines of what is done for <quantization/>.

jsbsim/src/models/flight_control/FGSensor.cpp

Line 78 in 13008f3

quant_property = quantization_element->GetAttributeValue("name");

jsbsim/src/models/flight_control/FGSensor.cpp

Lines 268 to 279 in 13008f3

    
           if (!quant_property.empty()) { 
        
             if (quant_property.find("/") == string::npos) { // not found 
        
               string qprop = "fcs/" + PropertyManager->mkPropertyName(quant_property, true); 
        
               FGPropertyNode* node = PropertyManager->GetNode(qprop, true); 
        
               if (node->isTied()) { 
        
                 XMLLogException err(fcs->GetExec()->GetLogger(), el); 
        
                 err << "Property " << tmp << " has already been successfully bound (late).\n"; 
        
                 throw err; 
        
               } 
        
               else 
        
                 PropertyManager->Tie(qprop, this, &FGSensor::GetQuantized); 
        
             }

But ended up going with the option of the user supplying the random seed value as an attribute of the <noise/> element instead.

But thinking about it a bit more, I'm inclined to switch to a scheme matching the <quantization/> approach.

seanmcleod70 · 2025-04-16T13:44:37Z

I've pushed some commits to switch to generating a sensor specific randomseed property. One of the builds failed due to a disk space issue, but all the other builds built successfully and the tests succeeded.

seanmcleod70 · 2025-04-19T19:51:54Z

Looks like, without a subsequent commit the build task that failed due to lack of disk space was re-run and now show all builds/tests are successful/green tick.

bcoconni

It seems to me that the way the property name for randomseed is determined is too complicated. I understand that you have duplicated the code from quant_property but I think there is a much simpler way to proceed (see my comments in the code).

bcoconni · 2025-04-21T11:22:49Z

src/models/flight_control/FGSensor.cpp

+  if (!noise_property.empty()) {
+    if (noise_property.find("/") == string::npos) { // not found
+      string nprop = "fcs/" + PropertyManager->mkPropertyName(noise_property, true) + "/randomseed";
+      FGPropertyNode* node = PropertyManager->GetNode(nprop, true);
+      if (node->isTied()) {
+        XMLLogException err(fcs->GetExec()->GetLogger(), el);
+        err << "Property " << tmp << " has already been successfully bound (late).\n";
+        throw err;
+      }
+      else
+        PropertyManager->Tie(nprop, this, &FGSensor::GetNoiseRandomSeed, &FGSensor::SetNoiseRandomSeed);
+    }
+  }


It seems too complicated to me, I'd rather duplicate the code of the malfunction/ properties:

const string tmp_randomseed = tmp + "/randomseed"; PropertyManager->Tie(tmp_randomseed, this, &FGSensor::GetNoiseRandomSeed, &FGSensor::SetNoiseRandomSeed);

bcoconni · 2025-04-21T11:24:31Z

src/models/flight_control/FGSensor.h

  <input> property </input>
  <lag> number </lag>
-  <noise [variation="PERCENT|ABSOLUTE"] [distribution="UNIFORM|GAUSSIAN"]> number </noise>
+  <noise [variation="PERCENT|ABSOLUTE"] [distribution="UNIFORM|GAUSSIAN"] [name="name"] > number </noise>


No need to add a name attribute to <noise>, we already know the name of the property: randomseed so we don't need to ask the user to provide one.

bcoconni · 2025-04-21T11:25:01Z

src/models/flight_control/FGSensor.h

  <input> aero/qbar </input>
  <lag> 0.5 </lag>
-  <noise variation="PERCENT"> 2 </noise>
+  <noise variation="PERCENT" name="qbarsensor"> 2 </noise>


Same as previously, you can remove it from the example.

bcoconni · 2025-04-21T11:34:34Z

Looks like, without a subsequent commit the build task that failed due to lack of disk space was re-run and now show all builds/tests are successful/green tick.

Yep, that was me ! I pushed the button "Re-run failed jobs" on GitHub and it managed to find a runner with sufficient disk space 😉

Removed the `name` attribute from the `<noise>` element in XML, simplifying noise configuration. Introduced a new `randomseed` property, automatically named based on the sensor's name. Updated `FGSensor::bind` to handle the new property and removed the obsolete `noise_property` member. Adjusted documentation, examples, and test cases to reflect these changes. This reduces complexity while preserving functionality for noise random seed configuration.

seanmcleod70 · 2025-04-21T16:38:57Z

Pushed a commit based on your suggestions. Copilot suggested commit message was pretty accurate so used it as is.

I pushed the button "Re-run failed jobs" on GitHub

I actually looked for that option, but couldn't find it. Could be because I'm not an 'Owner' of the repo, only a 'Member'.

bcoconni · 2025-04-21T17:09:55Z

PR merged 👍

I actually looked for that option, but couldn't find it. Could be because I'm not an 'Owner' of the repo, only a 'Member'.

I have promoted your account to "Owner" (and demoted your old account to "Member" because there is only one Sean 😉)

Co-authored-by: Sean McLeod <sean@seanmcleod.com>

seanmcleod added 3 commits April 7, 2025 23:02

Allow a sensor to specify a random seed for it's noise

7e3f760

Different exec seed compared to sensor seed

2d01fb0

Use tripod flight model in tests

5580ad3

seanmcleod added 5 commits April 16, 2025 14:26

Create sensor randomseed property

388b106

Merge branch 'master' into SensorRandomSeed

44948f2

Update exception logging

d30e5b0

Update test to use sensor randomseed property

4a361ac

Retrieve correct name

7a5b861

bcoconni requested changes Apr 21, 2025

View reviewed changes

bcoconni merged commit c36d074 into JSBSim-Team:master Apr 21, 2025
29 checks passed

bcoconni pushed a commit to bcoconni/jsbsim that referenced this pull request May 2, 2025

Sensor random seed (JSBSim-Team#1254)

315f34a

Co-authored-by: Sean McLeod <sean@seanmcleod.com>

Conversation

seanmcleod70 commented Apr 7, 2025

Uh oh!

codecov bot commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bcoconni commented Apr 13, 2025

Uh oh!

seanmcleod70 commented Apr 13, 2025

Uh oh!

bcoconni commented Apr 14, 2025

Uh oh!

seanmcleod70 commented Apr 14, 2025

Uh oh!

seanmcleod70 commented Apr 16, 2025

Uh oh!

seanmcleod70 commented Apr 19, 2025

Uh oh!

bcoconni left a comment

Choose a reason for hiding this comment

Uh oh!

bcoconni Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

bcoconni Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

bcoconni Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

bcoconni commented Apr 21, 2025

Uh oh!

seanmcleod70 commented Apr 21, 2025

Uh oh!

Uh oh!

bcoconni commented Apr 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Apr 8, 2025 •

edited

Loading