NRL-Plasma-Physics-Division / turbopy

A lightweight computational physics framework, based on the organization of turboWAVE. Implements a "Simulation, PhysicsModule, ComputeTool, Diagnostic" class hierarchy.
https://turbopy.readthedocs.io/
Creative Commons Zero v1.0 Universal
10 stars 18 forks source link

Reduce cyclomatic complexity of prepare_simulation #174

Closed kphlips closed 3 years ago

kphlips commented 3 years ago

Pull Request

Description

Reduce complexity of prepare_simulation method and write test that confirms the result is equivalent to running exchange_resources and initialize separately.

This pull request addresses # \ Fixes #

Checklist

The following items have been checked for this PR:

kphlips commented 3 years ago

I implemented the ID checks, and it seems that they are now different objects with different PhysicsModules. The modules are checked to have been prepared the same way through their string representations.

arichar6 commented 3 years ago

This is better. Now the two simulations are different.

However, they are very simple simulations, with only one PhysicsModule that doesn't have any complicated initialization logic.

If you want to really test this initialization logic, then you need to have a more complicated simulation. You should think about how to create a new test fixture with a simulation that checks that all of the modules have had their data shared before the initialize function is called.

kphlips commented 3 years ago

I changed the structure of the test so that it now uses a more complicated physics module. I think this is in the right direction towards what you were asking for.

arichar6 commented 3 years ago

Yes, this is going in the right direction. Now you need to override the initialize method in your AdvancedModule class, so that it uses a shared resource from another module. This means that you also need to override inspect_resource.

arichar6 commented 3 years ago

Does your code pass this new test?

kphlips commented 3 years ago

Not anymore

arichar6 commented 3 years ago

Can you create a PR to add this new test? As you discovered, there is an important feature of the code that is not being correctly tested yet.