Possible solution

[arichar6]

Pull Request

Description

Here's one possible "solution" to the sharing problem. I've made changes to the core classes (Simulation, PhysicsModule, and Diagnostic). I've also updated the block-on-spring example test so that it works with this new API.

In this approach, the "shared" resources are defined in custom PhysicsModules through two dictionaries: _resources_to_share and _needed_resources. The inspect_resources and exchange_resources functions have been changed. They now put the resources to share into the "global" shared dictionary, and then look in the global dictionary for the resources they need.

I think an approach like this could help developers as they write new "apps", since there are just the two dictionaries to define. They would no longer need to call publish_resource, or overload any of the functions related to exchanging resources.

Additionally, (as we discussed before), this opens the door to simpler error handling, and also things like code that could create a visual "map" of which modules share data and which use the shared data.

Thoughts?

[ndisner]

This does a good job a evaluating needed v shared. I don't know if this method will hold up yet in the bigger projects we have because you have the two dictionaries explicit represented in the block on spring problem. In our bigger problems we are continuously updating keys and refactoring that this could help us or hurt us (but this doesn't hurt to try).

[arichar6]

I'm thinking about making a "Release Candidate" version of turboPy using this sharing method. I know it will break code that uses the old sharing method, but it might be worth it. If we have an RC version of turboPy, then one of the projects that we can have interns work on is to update old apps to use the new sharing "API". Thoughts?

[ndisner]

Since the changes for this were bigger than expected yes I would say it would be good to have a compatible version with older code apps and that way interns or others (we) can work on transitioning apps to a better turboPy version. Starting with transitioning block-on-spring and particle-and-field before doing the rigid-beam-app.

[arichar6]

I think a good workflow could be this: 1) finish updating this branch to implement the new sharing 2) update any/all tests, and add new tests if needed 3) merge this PR into github, but don't create a new "release" yet (so pip install turbopy still gets the old version) 4) update example apps, and our research apps 5) once everything seems good/stable, make a new release. This should make a new version propagate out to pip, at which point anyone who updates to (or installs) the newest will have to update their code

[arichar6]

I'm looping @carolinegsun into this conversation, since she will be helping us finalize this work.

I know that I said that this code would "break" the old sharing method. However, I'm not sure that's entirely true. I'm curious if we could find a way to allow both the new API (using the _resources_to_share and _needed_resources dictionaries) alongside the old API (overloading the inspect/exchange_resources methods as needed).

It seems like there are (at least) three cases that we need to check:

1. All modules and diagnostics in a simulation use the new API
2. All modules and diagnostics in a simulation use the old API
3. Modules and diagnostics use a mix of old and new APIs (but each specific class uses only one or the other)

One additional case could be when a single PhysicsModule uses some aspects of each API. Say, it overloads exchange_resources method (old API) to tell other modules about data that it is sharing, but it defines the _needed_resources dictionary to look for and save data from other modules. I don't think we need to support this type of case. I think it's ok if we say that any given PhysicsModule or Diagnostic must use one or the other API.

There is one possible downside of keeping the old API along with the new API. Depending on how we implement it, there could be new features that will only work if your simulation uses only the new API. But maybe this downside won't cause too many problems.

[arichar6]

Maybe we can start by writing some tests for the three cases that need to be checked. I'm thinking about using some simple modules like in this core test.

We can have two which use the old API, e.g., ReceivingModuleV1 and SharingModuleV1, and two which use the new API, ReceivingModuleV2 and SharingModuleV2. Then we need tests for RV1 + SV1, RV2 + SV2, RV1 + SV2, and RV2 + SV1.

Does this make sense?

[carolinegsun]

Just wondering, in what case would RV1 + SV2 or RV2 + SV1 be integrated together? Otherwise, it makes sense to me.

[arichar6]

These cases could happen if someone wanted to use PhysicsModules that someone else already wrote with a different API. If we can make those cases work, then old code could be used without having to update it to the new API.

[codecov-commenter]

Codecov Report

Merging #170 (1daa0e2) into main (dc58f64) will decrease coverage by 0.52%. The diff coverage is 87.17%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
- Coverage   92.94%   92.41%   -0.53%     
==========================================
  Files           6        6              
  Lines         751      765      +14     
==========================================
+ Hits          698      707       +9     
- Misses         53       58       +5     

Impacted Files	Coverage Δ
turbopy/diagnostics.py	86.75% <71.42%> (-0.69%)	:arrow_down:
turbopy/core.py	92.70% <90.62%> (-0.91%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dc58f64...1daa0e2. Read the comment docs.