Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Convert params to hdf5 #723

Merged
merged 10 commits into from
Mar 19, 2024

Conversation

gtdang
Copy link
Collaborator

@gtdang gtdang commented Mar 8, 2024

  1. Created hnn-core function to convert param/json files to hdf5.
  2. Created new example param input files in HDF5 format for the tutorials.

Questions

  1. I converted all files found in the params folders of both hnn and hnn-core. Do we need all of them converted or should it be paired down?
  • Discussed in person. Each hdf5 param file is 1.3MB. Should be careful with adding too many. After working through the new tutorials we should determine which are no longer used and remove them from the repo.

closes #713

@gtdang gtdang changed the title Convert params to hdf5 [WIP] Convert params to hdf5 Mar 8, 2024
@gtdang gtdang requested a review from hollandjg March 8, 2024 19:31
@gtdang gtdang marked this pull request as ready for review March 11, 2024 15:08
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.64%. Comparing base (21dcea5) to head (d92fe8d).

❗ Current head d92fe8d differs from pull request most recent head 1d795fe. Consider uploading reports for the commit 1d795fe to get more accurate results

Files Patch % Lines
hnn_core/params.py 93.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #723   +/-   ##
=======================================
  Coverage   92.64%   92.64%           
=======================================
  Files          27       27           
  Lines        4893     4908   +15     
=======================================
+ Hits         4533     4547   +14     
- Misses        360      361    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

import tempfile


def download_folder_contents(owner, repo, path):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you pooch ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's stable and reduces the amount of code then perhaps it can be included under the [dev] install flag

But considering it would only remove this small function I'm inclined to keep this as is

@rythorpe
Copy link
Contributor

Are we sure we want to convert all current param files to hdf5? Since most the param files use one of 2 or 3 different network configurations, what if we simply convert those networks to hdf5 and use the API to specify the drives as set by the various param files?

On a broader note, the original param files were designed to specify both network and drive properties whereas the hdf5 files were designed to specify network properties. Of course, a network can contain drive properties as well, but I believe we should push users, particularly in tutorials, to load general-purpose network instances and then use the API to add drives. Maybe @jasmainak @ntolley disagree though?

@jasmainak
Copy link
Collaborator

I agree @rythorpe . Perhaps it's worth drawing a clear distinction between hnn_core.network_models and hdf5 files ... the former are template models whereas the latter is saving a particular state with particular connectivity having started from one of the template models. Making this distinction clear in the tutorials of hnn_core and HNN-GUI would be important.

hnn_core/io.py Outdated Show resolved Hide resolved
@gtdang
Copy link
Collaborator Author

gtdang commented Mar 12, 2024

@rythorpe
Are jones_2009_model, law_2021_model, calcium_model the 3 base network configs? So is your suggestion we convert those into hdf5 files just for the GUI tutorials and at the start of the tutorial have folks load network connectivity from one of those models?

Note that on GUI launch the default parameters are loaded, so there will already be a connectivity and set of drives present. Which base network config is the default based from? Should we be more explicit what the default actually is?

@rythorpe
Copy link
Contributor

@rythorpe Are jones_2009_model, law_2021_model, calcium_model the 3 base network configs? So is your suggestion we convert those into hdf5 files just for the GUI tutorials and at the start of the tutorial have folks load network connectivity from one of those models?

Not quite. The tutorials for the old GUI modeling gamma, alpha/beta, and evoked potentials (which we're now trying to replicate in the new GUI) all use variants of the jones_2009_model. If I recall correctly, the alpha/beta tutorial both use the default jones_2009_model while the gamma tutorial uses the same except that it disconnects L2/3 from L5. (@ntolley please correct me if I've missed something here.)

My suggestion is twofold: 1) for tutorials, always use the API to instantiate network models, and 2) build in the functionality for users to save/load hdf5 network instantiations if/when they wish to use them. Since the .param files distributed w/ hnn-core were designed to reproduce specific tutorial use-cases, we can discard them in favor of showcasing our API. The one exception is that we might want to make and distribute an hdf5 version of the "default" network (i.e., compiled from default.json/default.param) so that it can be used for testing purposes, since it can store the precise connectivity and drive times who's simulation output is compared to a "ground-truth" dipole waveform. If there's a strong logistical reason to also distribute a copy of the disconnected jones_2009_model used in the gamma tutorial (e.g., if we don't wish to expose the connectivity API needed to disconnect L2/3 from L5 in the GUI), than we can add that one too.

Note that on GUI launch the default parameters are loaded, so there will already be a connectivity and set of drives present. Which base network config is the default based from? Should we be more explicit what the default actually is?

Default is currently jones_2009_model + the evoked drive sequence due to its prevalence in prior publications as well as the tutorials, however, @stephanie-r-jones has recently recommended that we encourage users to start treating calcium_model as a good "default" starting point.

@gtdang
Copy link
Collaborator Author

gtdang commented Mar 12, 2024

@rythorpe
So if I understand correctly...

  • The GUI should have some sort of selection for the base network connectivity to use (this could be a dropdown or something).
  • The base networks would be alternatives of the jones_2009_model and/or calcium_model.
  • The base networks would exclude any drive information because we want to show how to add drives using the GUI UI.
  • The base networks won't be read-in by a file. Rather they will be constructed using the API on the fly. For example if the user selects "jones_2009_disconnected_l2l5" we'd have a function that just constructs and loads that type of network by loading from network_models and adjusting parameters.

Let me know if I have any of that wrong.

@rythorpe
Copy link
Contributor

@rythorpe So if I understand correctly...

* The GUI should have some sort of selection for the base network connectivity to use (this could be a dropdown or something).

I think this can be done with the "Load network connectivity" button on the "Network connectivity" tab, no?

* The base networks would be alternatives of the `jones_2009_model` and/or `calcium_model`.

At some point, yes. For now though, perhaps just getting the tutorials to work with jones_2009_model is the priority.

* The base networks would exclude any drive information because we want to show how to add drives using the GUI UI.

Yes!

* The base networks won't be read-in by a file. Rather they will be constructed using the API on the fly. For example if the user selects "jones_2009_disconnected_l2l5" we'd have a function that just constructs and loads that type of network by loading from network_models and adjusting parameters.

Yes!

@gtdang
Copy link
Collaborator Author

gtdang commented Mar 13, 2024

* The GUI should have some sort of selection for the base network connectivity to use (this could be a dropdown or something).

I think this can be done with the "Load network connectivity" button on the "Network connectivity" tab, no?

That is currently a file load button. So you would need to load a file like hdf5. If we move towards using just the API to load base networks this would have to be converted to a selector (likely dropdown) for a limited set of base networks.

In summary here's one potential solution:

  1. On the Network Connectivity tab window:
    • A dropdown-selector for a set of pre-defined base connectivity configurations
    • A confirm button to load the selected base connectivity
    • These replace the current "Load local network connectivity" button. As users won't be loading only connectivity from file.
  2. On the External Drives tab window:
    • The current "Load external drives" can be removed as users won't be loading only drives from file.
  3. New "Load saved network" button
    • This button will load both connection and drives from an hdf5 file. There is no option to load just connections or drives--it will be all-in-one.
    • This gives the user the ability to save configuration state of a tutorial and return back to it. It also allows us to supply a configuration state mid-tutorial if needed.
      • Note that this does not save the state of the simulations or plots generated

@rythorpe
Copy link
Contributor

I really like that solution. Perhaps we can pitch it to Stephanie and the team at our next developer's mtg to get their take because we really want to make sure it will work smoothly for workshops, etc.

@gtdang
Copy link
Collaborator Author

gtdang commented Mar 15, 2024

@rythorpe
Chatted with Nick yesterday and we decided the hdf5 method would be the easiest to meet our Spring deadlines for working tutorials. The implementation of a subset of API-derived base networks can hopefully be implemented in future. Its implementation should be fairly straight-forward with the underlying changes we are making with the GUI for hdf5 support. I've made a new ENH issue #729 that points back to this thread and some diagrams I've mocked up for the implementation.

For this current PR I'm only going to add the default.hdf5 to the params directory. Other param file hdf5 conversions will be contained in a separate repo hnn-data.

@gtdang gtdang changed the title [WIP] Convert params to hdf5 [MRG] Convert params to hdf5 Mar 15, 2024
Copy link
Contributor

@ntolley ntolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gtdang this looks great to me! happy to merge if you're all set on your end

@gtdang
Copy link
Collaborator Author

gtdang commented Mar 19, 2024

All set on my end! Go for it!

@ntolley ntolley merged commit 54ddd40 into jonescompneurolab:master Mar 19, 2024
9 checks passed
@ntolley
Copy link
Contributor

ntolley commented Mar 19, 2024

Great work @gtdang!! 🎉 🎉

@gtdang gtdang deleted the convert-params-to-hdf5 branch August 29, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GUI new tutorial input files
5 participants