PR for issue BSK-1164: Monte Carlo dispersions application

[ruthubotica]

• Tickets addressed: Monte Carlo dispersions are not being applied #1164
• Review: By commit
• Merge strategy: Merge (no squash)

Description

The setting of attributes is changed from using the simple setattr() command to using a function that loops through the elements of a chained attribute and correctly applies the dispersion. Additionally, a function is added to correctly retrieve the attributes (instead of using getattr()). Finally, a simParams input called recordSimParams (default value set to False) is added. If set to True, a JSON file can be created with the values of the attributes after the dispersion application, to verify that they have been applied. See the corresponding issue for more details.

Some additional details on the commits and the added functions (in the order that they appear in the code files):

Commit 1: Controller.py

• import additional required packages.
• add setRecordSimParams function to be able to edit the value of the recordSimParams attribute. The default value is set to False.
• set the recordSimParams attribute to false in the getParameters function.
• change the command that sets the attribute from setattr() to using the new function that works with nested attributes setNestedAtt.
• if recordSimParams is True, generate a JSON file with the values of the simulation run's attributes.
• add a static method called setNestedAtt() to methodically and correctly use nested attributes.
• add a static method called getNestedAtt() to methodically and correctly get the nested attributes, to be used for recording the simulation run's attributes if recordSimParams is set to True.

Commit 2: scenarioBskSimAttFeedbackMC.py

• add recordSimParams as an input to the scenario's run() function, with a default value set to False. This is added so that the attribute can be set to True when running the test function.
• add a line to explicitly set the attribute of the Controller() instance to the value inputted into the run() function.

Commit 2: test_bskMcTestScript.py

• add the required additional import statements
• add the test function test_dispersionApplicationMc(), which runs an instance of the scenarioBskSimAttFeedbackMC with recordSimParams set to True. The JSON files containing the simParams (which are actually the values from the dispersions modifications list) and the recorded attributes of run 0 are compared. The files are then cleaned up.

Verification

A test function has been added to the test_bskMcTestScript.py file. The output of the simParams JSON file for run 0 of scenarioBskSimAttFeedbackMC is compared with the identical format JSON file using the recordSimParams functionality to record the dispersed attributes. As mentioned also in the issue: please note that this scenario does not currently pass this test, since even with the proposed solution some attributes are still not being dispersed. I have not been able to find the reason why this is happening, but I assume it is due to the way in which the modules are being used in this specific scenario, since the scenarios I have made for my project all pass the test.

Documentation

The documentation on the examples of Monte Carlo scenarios must be updated. The plots are incorrect since they show un-dispersed values (see also the information in the issues ticket).

[schaubh]

Howdy @ruthubotica , thanks for looking to contribute to this project. however, I don't think this branch is ready for a PR at this point as it is not a complete implementation?

Also, you are saying the MC runs don't do a proper variable dispersion. Are you sure this is not a setup error? If you are getting the same values each time, are you not providing the random number generator a unique seed?

[schaubh]

The build errors appear to be related to libpng not installing correct. I'll have to see if this is happening across other branches as well. Could be a conan server issue.

[ruthubotica]

Hi! My conclusion that is an issue with the Controller() class and not a setup error is mainly based on the fact that if I run the example scripts (having set up Basilisk according to the instructions and such and not editing any source code) the values across runs are the same. For example, if scenarioMonteCarloAttRW.py is run enabling Bokeh with the current code in the repository, it is very clear on the Bokeh page that the values are the same for the runs. In the documentation of scenarioVisualizeMonteCarlo.py the Bokeh figures also only show one line, which is 4 separate plots on top of each other with the same values. So this is why I assumed it is a source code error and not just an implementation issue on my own local version and my personal project files.

However if I have missed something please do let me know of course!

[schaubh]

In these scripts the same seed is being provided for each run.

[schaubh]

Regarding the build issues, did you rebase your branch on the latest BSK develop?

[ruthubotica]

Regarding the build issues, did you rebase your branch on the latest BSK develop?

I made sure it was up to date yesterday before making the branch and pushing the changes, so it should have been yes.

Also just updated the branch with the latest changes since yesterday, so should definitely be up to date now :)

[conor-ubotica]

Howdy @ruthubotica , thanks for looking to contribute to this project. however, I don't think this branch is ready for a PR at this point as it is not a complete implementation?

Also, you are saying the MC runs don't do a proper variable dispersion. Are you sure this is not a setup error? If you are getting the same values each time, are you not providing the random number generator a unique seed?

Yes, the issue that we identified is with the mechanism for applying dispersions. It's related in particular to the use of setattr to apply the dispersions. Setattr doesn't support nested attributes and in many cases this means the dispersions aren't successfully applied. @ruthubotica added a setNestedAtt function that addresses this issue and some supporting test code to validate the problem and the fix. In this particular case, I don't think that the issue is related to the seeding of the number generator.