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

Enable users to remove gid from filename in recording devices #760

Merged
merged 5 commits into from Jul 19, 2017

Conversation

@weidel-p
Copy link
Contributor

weidel-p commented Jun 20, 2017

Hi,

with this PR I'd like to give the user more power over the filename of recording_devices. So far, the GID of the device is a obligatory part of the filename. This can be annoying during the development of network models as the GID might change quite often.

I introduced a field called use_gid as a parameter of the recording_device which, if turned off, makes the recording_device neglect its GID in the filename. As this can lead to more collisions of filenames, this mechanism only works if the user has set a prefix for the filename.

Copy link
Contributor

abigailm left a comment

all good except for the critical bit.

<< node_.get_gid() << "-" << std::setfill( '0' )
<< std::setw( vpdigits ) << node_.get_vp();
}
else

This comment has been minimized.

@abigailm

abigailm Jun 29, 2017 Contributor

not P_.label.empty() needs to be part of the else branch. Perhaps reverse the order.

@abigailm
Copy link
Contributor

abigailm commented Jun 29, 2017

👍

Copy link
Contributor

jougs left a comment

I'm a bit sceptical that this change is a good idea. For example, what would happen if a user sets the same label for two devices on the same VP? Will NEST crash? Or will the data in the file be corrupted? I'm afraid that this leads to hard-to-debug errors as we can't easily guarantee that the labels are unique. Can you please test these kinds of situations and report back?

Also, the feature needs documentation in the SLI docstring of RecordingDevice and possibly also somewhere in the NEST user manual.

For more general remarks on the code see my inline comments.

@@ -484,6 +484,7 @@ const Name U_std( "U_std" );
const Name U_upper( "U_upper" );
const Name update( "update" );
const Name update_node( "update_node" );
const Name use_gid( "use_gid" );

This comment has been minimized.

@jougs

jougs Jun 29, 2017 Contributor

This name should refer to the fact that the gid is used in the filename. As it is now, this does not become clear from the variable name

@@ -549,6 +549,7 @@ extern const Name U_std;
extern const Name U_upper;
extern const Name update; //!< Command to execute the neuron (sli_neuron)
extern const Name update_node; //!< Command to execute the neuron (sli_neuron)
extern const Name use_gid; //!< use gid in the filename

This comment has been minimized.

@jougs

jougs Jun 29, 2017 Contributor

See above.

@@ -226,6 +229,8 @@ nest::RecordingDevice::Parameters_::set( const RecordingDevice& rd,
updateValue< bool >( d, names::flush_records, flush_records_ );
updateValue< bool >( d, names::close_on_reset, close_on_reset_ );

updateValue< bool >( d, names::use_gid, use_gid_ );

This comment has been minimized.

@jougs

jougs Jun 29, 2017 Contributor

This line requires to assign to a temporary first and only after checking if a label is set put it in the global variable. If use_gid is set to true by the user and no label is set, a BadProperty exception should be thrown with an appropriate error message.

@weidel-p weidel-p force-pushed the weidel-p:feature/use_gid branch from fbed29a to 0035b47 Jun 30, 2017
@weidel-p
Copy link
Contributor Author

weidel-p commented Jun 30, 2017

I renamed use_gid to use_gid_in_filename, extended the docstring and added a BadPropertyException.

If a user sets the same label for different devices and sets use_gid_in_filename to false, NEST first throws the usual IOError:

The device file 'spike_detector-0.gdf' exists already and will not be  overwritten. 
Please change data_path, data_prefix or label, or set 
/overwrite_files to true in the root node.

The data only gets corrupted, if the user sets overwrite_files to true.

I understand your concerns and also see the possible risks in this feature but I think the user is well enough protected by being forced to give a label and by the IOError.

@jougs
Copy link
Contributor

jougs commented Jul 11, 2017

Sorry to be nagging about this, but: please rename all occurences of use_gid to use_gid_in_filename. Otherwise this gets really confusing and the function of this variable is not clear outside of the recording context. Thanks!

weidel-p added 2 commits Jul 18, 2017
@weidel-p
Copy link
Contributor Author

weidel-p commented Jul 18, 2017

@jougs I changed every occurrence of use_gid to use_gid_in_filename. Additionally, I added two tests to this PR.

@jougs
jougs approved these changes Jul 19, 2017
Copy link
Contributor

jougs left a comment

@weidel-p: many thanks for the fixes. The code now looks good to me and I'l merge, given that @abigailm already approved.

@jougs jougs merged commit 1df2ff0 into nest:master Jul 19, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.