[MRG] Set legacy mode to false by default #619

ntolley · 2023-03-13T17:25:43Z

closes #526

Changes all network models to use legacy_mode=False by default. Some minor changes in tests/examples were necessary but overall a pretty small change.

ntolley · 2023-03-13T22:13:56Z

Unfortunately #614 didn't solve all our problems since the GUI has it's own code to loop over drives and add them. I've added the same logic to skip over these drives.

Not really sure what the best course of action is. @chenghuzi @jasmainak it seems like this will be necessary for loading old param files? Unless we enforce that you need to delete drive elements that contain invalid values.

jasmainak · 2023-03-14T03:24:38Z

sounds like the logic of legacy_mode that you implemented here should actually go inside _extract_drive_specs_from_hnn_params ? So it will return only the non-legacy drives?

chenghuzi · 2023-03-15T00:34:09Z

sounds like the logic of legacy_mode that you implemented here should actually go inside _extract_drive_specs_from_hnn_params ? So it will return only the non-legacy drives?

Could we have a more structured way of creating networks?

rythorpe · 2023-03-15T01:59:18Z

What do you propose? At some point we need to cut out all the param file bloat (e.g., consolidate params.py and params_default.py), but the work just hasn't been done yet primarily because of our dependence on legacy_mode.

ntolley · 2023-03-15T17:40:15Z

hnn_core/tests/test_gui.py

@@ -69,7 +69,6 @@ def test_gui_upload_params():
    assert gui.drive_widgets[-1]['tstop'].value == 250.
    gui._simulate_upload_drives(file1_url)
    assert gui.connectivity_widgets[0][0].children[1].value == 0.01
-    assert gui.drive_widgets[-1]['tstop'].value == 0.


This test does not apply anymore, the poisson drive (that is not used, hence tstop=0) is no longer added when legacy_mode=False

ntolley · 2023-03-15T17:45:08Z

Ok I've moved all of the logic into _extract_drive_specs_from_hnn_params so hopefully that's the last of it!

hnn_core/params.py

hnn_core/tests/conftest.py

hnn_core/params.py

hnn_core/network_models.py

ntolley · 2023-03-15T22:31:29Z

hnn_core/network.py

@@ -353,6 +355,12 @@ def __init__(self, params, add_drives_from_params=False, legacy_mode=True):
        # XXX this can be removed once tests are made independent of HNN GUI
        # creates nc_dict-entries for ALL cell types
        self._legacy_mode = legacy_mode
+        if self._legacy_mode:
+            warnings.warn(
+                'Legacy mode is used solely to maintain compatibility with'


@jasmainak what do you think of this? I'm ok with being annoying if you are 😉

okay for me ! what happens during optimization though? Hope the warning is emitted only once?

That's a good point - I'm assuming this will get printed anytime Network.copy() is called?

Alternatively, we could place this warning in jones_2009_model() since all the other template models call that function

My reasoning was they all have legacy mode as well as an option

ntolley · 2023-03-15T22:35:17Z

Just remembered I need to double check how badly the beta example is impacted by this before this PR is set to merge (it's fairly sensitive to the random seed)

ntolley · 2023-03-16T02:21:43Z

hnn_core/tests/test_drives.py



 def test_external_drive_times():
    """Test the different external drives."""

-    params = Params()
+    params_fname = op.join(hnn_core_root, 'param', 'gamma_L5weak_L2weak.json')


The test below assumes the existence of a poisson drive, which only gets added if legacy_mode=True for default.json

You might be able to get away with removing this read_params() step along with _extract_drive_specs_from_hnn_params by defining the poisson drive dynamics on L38 explicitly. I think this would be a cleaner unit test that circumvents the black magic contained in _extract_drive_specs_from_hnn_params.

ntolley · 2023-03-16T02:23:41Z

Just checked the beta example locally and it isn't impacted! I'll keep an eye on lingering CI's but I can see the legacy mode cord being cut very soon 😄

hnn_core/network.py

rythorpe · 2023-03-16T03:26:31Z

Also, looks like something is now off with the seed values that causes test_parallel_backend to fail. I'm guess that a negative seed value gets read in from the .param file which then doesn't get the proper gid-boost that it used to from the ghost poisson drives?

jasmainak · 2023-03-16T20:52:43Z

This is huge @ntolley !! Don't forget to document in whats_new.rst !!

Co-authored-by: Ryan Thorpe <ryvthorpe@gmail.com>

rythorpe · 2023-03-19T23:39:21Z

Do you want help with this @ntolley in order to be ready for tomorrow?

ntolley · 2023-03-19T23:41:30Z

If you have the time to check out the seed problem that'd be great! I'm literally troubleshooting my MPI build right now

jasmainak · 2023-03-20T01:11:04Z

I updated the PR description to close #526 !

…ode warning

… dynamics dict

…gacy_mode is True

rythorpe · 2023-03-20T04:40:39Z

hnn_core/params.py

+            if legacy_mode:
+                drive['event_seed'] -= 18


This was one of the culprits causing test_parallel_backend.py to fail.

Great find!!

rythorpe · 2023-03-20T04:44:18Z

hnn_core/tests/conftest.py

+            legacy_mode = False
+        # Legacy mode necessary for exact dipole comparison test
+        net = jones_2009_model(params, add_drives_from_params=True,
+                               legacy_mode=legacy_mode)


The other culprit was here. When test_parallel_backend.test_mpi_failure() was called, it didn't expect to encounter the new legacy mode deprecation warning. The easiest way around this was to just set legacy_mode=False when running the reduced model simulation.

hnn_core/tests/test_drives.py

codecov-commenter · 2023-03-20T05:05:40Z

Codecov Report

Merging #619 (0557bcc) into master (d5d1a03) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head 0557bcc differs from pull request most recent head 07f6574. Consider uploading reports for the commit 07f6574 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
- Coverage   92.19%   92.04%   -0.15%     
==========================================
  Files          22       22              
  Lines        4229     4238       +9     
==========================================
+ Hits         3899     3901       +2     
- Misses        330      337       +7

Impacted Files	Coverage Δ
hnn_core/drives.py	`98.54% <ø> (+0.66%)`	⬆️
hnn_core/gui/gui.py	`95.02% <ø> (-1.43%)`	⬇️
hnn_core/network.py	`93.39% <100.00%> (+0.04%)`	⬆️
hnn_core/network_models.py	`100.00% <100.00%> (ø)`
hnn_core/params.py	`91.89% <100.00%> (+0.25%)`	⬆️
hnn_core/viz.py	`89.23% <100.00%> (+0.04%)`	⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jasmainak · 2023-03-20T15:56:17Z

Thanks @ntolley @rythorpe ! 🥳

ntolley added 3 commits March 13, 2023 12:41

Set legacy mode to false by default

92025b4

hnn_core fixture uses legacy mode

d5cad0e

Skip invalid drives in GUI

7f19e02

Move legacy mode logic to extract_drives_from_params and fix test

b75dd4a

ntolley commented Mar 15, 2023

View reviewed changes

ntolley changed the title ~~WIP: Set legacy mode to false by default~~ [MRG] Set legacy mode to false by default Mar 15, 2023

rythorpe reviewed Mar 15, 2023

View reviewed changes

hnn_core/params.py Outdated Show resolved Hide resolved

rythorpe reviewed Mar 15, 2023

View reviewed changes

hnn_core/tests/conftest.py Outdated Show resolved Hide resolved

jasmainak reviewed Mar 15, 2023

View reviewed changes

hnn_core/params.py Show resolved Hide resolved

jasmainak reviewed Mar 15, 2023

View reviewed changes

hnn_core/network_models.py Show resolved Hide resolved

Fix legacy_mode passing, add deprecation warning

0fb2981

ntolley commented Mar 15, 2023

View reviewed changes

ntolley changed the title ~~[MRG] Set legacy mode to false by default~~ WIP: Set legacy mode to false by default Mar 15, 2023

Fix drives test that assumed legacy mode

8c1f43a

ntolley changed the title ~~WIP: Set legacy mode to false by default~~ [MRG] Set legacy mode to false by default Mar 16, 2023

flake8

3f25fa4

ntolley commented Mar 16, 2023

View reviewed changes

rythorpe reviewed Mar 16, 2023

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

Update hnn_core/network.py

8c93a93

Co-authored-by: Ryan Thorpe <ryvthorpe@gmail.com>

jasmainak mentioned this pull request Mar 20, 2023

make drive event times independent of gid (randomisation) #234

Closed

rythorpe added 3 commits March 20, 2023 00:12

TST: set legacy_mode=False in reduced model test to suppress legacy m…

932acae

…ode warning

TST: try simplifying test_external_drive_times by explicitly defining…

617dcc6

… dynamics dict

BUG: only reduce seed in _extract_drive_specs_from_hnn_params when le…

c44be70

…gacy_mode is True

rythorpe reviewed Mar 20, 2023

View reviewed changes

hnn_core/tests/test_drives.py Show resolved Hide resolved

TST: remove unnecessary import in test_drives.py

211eb36

ntolley added 4 commits March 20, 2023 10:19

Remove extgauss from params

07f6574

Better logic for extgauss

495ca96

Update whats new

c95869b

put back extgauss logic :(

74125fd

jasmainak enabled auto-merge (rebase) March 20, 2023 14:52

jasmainak merged commit 9bc2230 into jonescompneurolab:master Mar 20, 2023
7 checks passed

ntolley mentioned this pull request Apr 17, 2023

[MRG] Add tonic inputs #635

Open

jasmainak mentioned this pull request Jun 26, 2023

[WIP] MAINT: deprecate legacy_mode #533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Set legacy mode to false by default #619

[MRG] Set legacy mode to false by default #619

ntolley commented Mar 13, 2023 •

edited by jasmainak

ntolley commented Mar 13, 2023

jasmainak commented Mar 14, 2023

chenghuzi commented Mar 15, 2023

rythorpe commented Mar 15, 2023

ntolley Mar 15, 2023

ntolley commented Mar 15, 2023

ntolley Mar 15, 2023

jasmainak Mar 16, 2023

rythorpe Mar 20, 2023

rythorpe Mar 20, 2023

ntolley Mar 20, 2023

ntolley commented Mar 15, 2023

ntolley Mar 16, 2023 •

edited

rythorpe Mar 16, 2023

ntolley commented Mar 16, 2023

rythorpe commented Mar 16, 2023

jasmainak commented Mar 16, 2023

rythorpe commented Mar 19, 2023

ntolley commented Mar 19, 2023

jasmainak commented Mar 20, 2023

rythorpe Mar 20, 2023

ntolley Mar 20, 2023

rythorpe Mar 20, 2023

codecov-commenter commented Mar 20, 2023 •

edited

jasmainak commented Mar 20, 2023

[MRG] Set legacy mode to false by default #619

[MRG] Set legacy mode to false by default #619

Conversation

ntolley commented Mar 13, 2023 • edited by jasmainak

ntolley commented Mar 13, 2023

jasmainak commented Mar 14, 2023

chenghuzi commented Mar 15, 2023

rythorpe commented Mar 15, 2023

Choose a reason for hiding this comment

ntolley commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntolley commented Mar 15, 2023

ntolley Mar 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntolley commented Mar 16, 2023

rythorpe commented Mar 16, 2023

jasmainak commented Mar 16, 2023

rythorpe commented Mar 19, 2023

ntolley commented Mar 19, 2023

jasmainak commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 20, 2023 • edited

Codecov Report

jasmainak commented Mar 20, 2023

ntolley commented Mar 13, 2023 •

edited by jasmainak

ntolley Mar 16, 2023 •

edited

codecov-commenter commented Mar 20, 2023 •

edited