[WIP] GUI hdf5 parameter loading #722
Conversation
|
@ntolley @jasmainak I've gotten to the point of being able to read in the hdf5 file as a Network object and now I need to get it into the same dict format of self.param so I can update the GUI's parameter state. Is there a way to create a param dict/json from a Network object? If not I'll need to come up with a way to map all of a network's attributes to the correct key-value pairs. |
|
@gtdang unfortunately I think the latter option is necessary. The reason we developed the hdf5 format is precisely because we didn't have a way to output HNN-core network models into a |
|
Can you point me to a line where this crops up? It may be more expedient for me to figure out that mapping since I have a good sense of what attributes in the Network object map to |
|
@ntolley The GUI functions are often initializing a Network object with the params attribute and then uses the Network object in another function to display the encoded Connections and Drives. I suppose if we wanted to do away with the params attribute we could just load the HDF5 Network to the GUI and pass that around. The function that would need to be reworked is the
|
One solution to support both hdf5 and the param file is:
|
|
@gtdang the more complexity you add, the more maintenance work and the harder for a future contributor ... I think you guys need to take a judgement call whether to support hdf5 or param or both and whether for read/write or both. My plan was to phase out params by outputting hdf5 always (never params/json) but allowing to read json for backwards compatibility ... for a while. |
Totally agree with keeping complexity at a minimum. I'm curious about the shift to hdf5. Is the plan for the Network to only be instantiated by an hdf5 read-in? Seems like a chicken and egg situation where you need a Network to write to hdf5 yet need to init a Network with an hdf5. Is the idea that users are not likely create & edit their own hdf5 Network and are always starting with pre-constructed Networks? Do we have a recommendations for hdf5 viewers/editors for those that want to edit params outside of the the API? |
|
We don't want to encourage users to edit params outside the API ... they are on their own if they do so. In the past, we had the params being passed around willy-nilly in different functions and it was hard to keep track what was happening. Modifying the Network only through the API allows us to validate the parameters while also ensuring that they are consistent ... for example start/end time of the drives cannot be outside the simulation start/end time. Hdf5 was chosen as it's a simple hierarchical format that allows you to translate python objects into a binary format while also being future-proof (unlike pickling). Users should always start with a template network that we provide and make changes through the API. They can then save the network and load it back but they cannot make arbitrary changes to the network object. |
dev_scripts/convert_params.py
Outdated
| output_filenames = [Path(output_dir, f.name.split('.')[0]) | ||
| for f in file_list] | ||
|
|
||
| # Conversion | ||
| [convert_to_hdf5(file, outfile) | ||
| for (file, outfile) in zip(file_list, output_filenames)] |
There was a problem hiding this comment.
This looks like it might be simpler just as a for loop over for file in file_list:.
dev_scripts/convert_params.py
Outdated
| print(f"Failed to retrieve contents. Status code: " | ||
| f"{response.status_code}") |
There was a problem hiding this comment.
Should this raise an exception instead?
You could use this pattern: https://stackoverflow.com/a/24531618
... which might simplify your code a bit.
hnn_core/params.py
Outdated
| def _convert_to_path(value): | ||
| if isinstance(value, str): | ||
| value = Path(value) | ||
| return value |
There was a problem hiding this comment.
Do you need this function? Could you just do Path(value) wherever you use this function? Or do you rely on it not throwing an error and just returning None if you give it, say, an integer?
There was a problem hiding this comment.
Looking further, it looks like you might pass it a Path instead. Path is meant to be idempotent, so that should still be fine.
Path throws an error unless you give it a string or a Pathlike-object (something with a __fspath__ method which returns a path on your system).
hnn_core/params.py
Outdated
| _validate_type(params_fname, (str, Path), 'params_fname') | ||
| _validate_type(out_fname, (str, Path), 'out_fname') | ||
|
|
||
| params_fname = _convert_to_path(params_fname) | ||
| out_fname = _convert_to_path(out_fname) |
There was a problem hiding this comment.
Could you just do:
| _validate_type(params_fname, (str, Path), 'params_fname') | |
| _validate_type(out_fname, (str, Path), 'out_fname') | |
| params_fname = _convert_to_path(params_fname) | |
| out_fname = _convert_to_path(out_fname) | |
| params_fname = pathlib.Path(params_fname) | |
| out_fname = pathlib.Path(out_fname) |
That'll validate the types of the variables (and throw the same TypeError if they're incompatible) and make your Path objects.
hnn_core/tests/test_params.py
Outdated
| net_hdf5 = read_network(outpath) | ||
| assert net_hdf5 == net_params | ||
|
|
||
| # Check if writing with no extension will add one |
There was a problem hiding this comment.
| # Check if writing with no extension will add one | |
| # Check that writing with no extension will add one |
| # Check if writing with no extension will add one | ||
| outpath_no_ext = Path(tmp_path, 'default_no_ext') | ||
| convert_to_hdf5(params_base_fname, outpath_no_ext) | ||
| assert outpath_no_ext.with_suffix('.hdf5').exists() |
There was a problem hiding this comment.
You could also include the comment in the assert statement, so if it fails you get a more useful error message
| assert outpath_no_ext.with_suffix('.hdf5').exists() | |
| assert outpath_no_ext.with_suffix('.hdf5').exists(), "Writing with no extension adds one" |
hnn_core/tests/test_params.py
Outdated
| def test_convert_to_hdf5_bad_type(): | ||
| """Tests type validation in convert_to_hdf5 function""" | ||
| good_path = hnn_core_root | ||
| path_str = good_path.__str__() |
There was a problem hiding this comment.
You could use the str function rather than the dunder method here – it should do the same thing and seems a bit more standard than calling the dunder method directly (though I can't find a good reference for that – just some posts on StackExchange).
| path_str = good_path.__str__() | |
| path_str = str(good_path) |
dev_scripts/convert_params.py
Outdated
| @@ -0,0 +1,93 @@ | |||
| """Script converting legacy param and json parameter files to hdf5. | |||
There was a problem hiding this comment.
Is this PR still relevant. I see: https://github.com/jonescompneurolab/hnn-core/tree/master/dev_scripts already in main ? Or does it need a rebase?
There was a problem hiding this comment.
It's currently held up by #733. Will be rebased after that one is merged.
…son file source to write function
…e is from a param file
… and bursty drives
…hdf5 and handling during read-in
…oved _connectivity_to_list_of_dicts function and call _conn_to_dict directly in write_network.
…read_params already checks.
* Changed to load files into memoryview instead of bytes as this is the format consistent with ipywidgets documentation and was not working for hdf5. * Added different mimetypes for different types of files.
…o methods to declutter init.
|
Closing this because hdf5 files in the current format are too large. We are working on a different solution for reading/writing outputs. |
Add support for loading connectivity and drive parameters from an hdf5 saved network object.
closes #708