[MRG] fix inconsistent random state assignment between parallel trials

[rythorpe]

fixes #136

Running the code here creates this plot with MPI_Backend:

and this plot for JoblibBackend:

Note that while individual trials are not exactly the same between the two versions (most likely due to an inconsistency in the trajectories of random states), both show 10 distinct trials that emerged from distinct random states. If anyone can readily spot where the discrepancy is occurring it be much appreciated. Otherwise we might want to address that in another PR since the tests are still passing.

This PR also addresses some of the inconsistent messaging that gets output during a simulation in the terminal.

[jasmainak]

is this equivalent to https://github.com/jonescompneurolab/hnn-core/pull/171/files#diff-ca5f146e027d9dc6ef29ff4db9c21ca2R68 ? I have a suspicion it is not. Can you check?

[rythorpe]

You're right, I messed this up. Fixed in b340fe0.

[jasmainak]

should we add an "XXX" in the comment somewhere? To indicate that this is a hack (right?) and we should get rid of at some point

[rythorpe]

Hmm, I'm not sure in this scenario what delineates a hack from a long-term bug fix. We plan to refactor the params and feed modules wrt how feed objects and their respective parameters are passed to Network and NetworkBuilder. Along the way we will end up simplifying the code by e.g. removing the need for calling create_pext() here. That said, I'm happy to update the comment if you think it'd be clearer.

[jasmainak]

yeah, I agree it might go away but just leaving an XXX tells future contributors that it's a hack that needs to fixed.

[jasmainak]

I have a hunch that digging into this might solve your issue. Then ideally, we can add a tiny test to check that MPI vs Joblibs gives same result for 2 trials (with a 3 x 3 network to make it run fast). If it doesn't work, I am fine merging the PR as is if it's good by @blakecaldwell

do make sure to update whats_new.rst. I see that you found a fun way to end your week ;-)

[codecov-commenter]

Codecov Report

Merging #171 into master will increase coverage by 0.88%.
The diff coverage is 35.71%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   67.61%   68.50%   +0.88%     
==========================================
  Files          19       19              
  Lines        1930     1959      +29     
==========================================
+ Hits         1305     1342      +37     
+ Misses        625      617       -8     

Impacted Files	Coverage Δ
hnn_core/mpi_child.py	0.00% <0.00%> (ø)
hnn_core/parallel_backends.py	16.41% <0.00%> (ø)
hnn_core/network_builder.py	91.74% <100.00%> (+3.16%)	⬆️
hnn_core/cell.py	69.23% <0.00%> (-5.66%)	⬇️
hnn_core/pyramidal.py	98.32% <0.00%> (-0.10%)	⬇️
hnn_core/basket.py	100.00% <0.00%> (ø)
hnn_core/tests/test_cell.py	100.00% <0.00%> (ø)
hnn_core/tests/test_network.py	100.00% <0.00%> (ø)
hnn_core/params.py	61.37% <0.00%> (+6.20%)	⬆️

---

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 04ab8bb...b340fe0. Read the comment docs.

[rythorpe]

Update with b340fe0: the script that generated the plots shown at the beginning of this PR now produces the same results for the joblib and mpi backends when running the default param set:

[jasmainak]

Amazing! do you think we could add a tiny test?

• perhaps one test that checks that the two trials are different
• and another that checks that the results are consistent between joblib and MPI?

you can use a small network, e.g., params.update({"N_pyr_x": 3, "N_pyr_y": 3}) and maybe a shorter tstop to make it run fast?

also don't forget to update whats_new.rst!

[rythorpe]

I've updated whats_new.rst and added your suggested tests @jasmainak. I also ended up rearranging some of the test functions in test_compare_hnn.py in b8f5d1d in order to limit redundant code. Take a look and let me know if the changes are clear and acceptable.

[jasmainak]

Fantastic, thanks for making the changes. PR looks good to me, merged!