-
Notifications
You must be signed in to change notification settings - Fork 391
PyNEST-NG: A new Python interface for NEST directly connecting Python to C++ #3524
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
base: master
Are you sure you want to change the base?
Conversation
…d map iteration from C++11 to C++17.
jessica-mitchell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor fixes
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
| @@ -27,14 +27,12 @@ | |||
| #include <limits> | |||
| #include <vector> | |||
|
|
|||
| #include <boost/any.hpp> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this NEST depends on boost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing pyNEST folder (except examples subfolder) -> LGTM!
Please see several code suggestions inline.
| @@ -362,7 +301,7 @@ def get_help_fname(obj): | |||
| File name of the help text for obj | |||
| """ | |||
|
|
|||
| docdir = sli_func("statusdict/prgdocdir ::") | |||
| docdir = nestkernel.ll_api_get_kernel_status()["docdir"] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| docdir = nestkernel.ll_api_get_kernel_status()["docdir"] | |
| docdir = nestkernel.llapi_get_kernel_status()["docdir"] |
| @@ -80,7 +57,7 @@ def helpdesk(): | |||
|
|
|||
| """ | |||
|
|
|||
| docdir = sli_func("statusdict/prgdocdir ::") | |||
| docdir = nestkernel.ll_api_get_kernel_status()["docdir"] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| docdir = nestkernel.ll_api_get_kernel_status()["docdir"] | |
| docdir = nestkernel.llapi_get_kernel_status()["docdir"] |
| @@ -574,9 +569,10 @@ def __bool__(self): | |||
| """Converts the NodeCollection to a bool. False if it is empty, True otherwise.""" | |||
| return len(self) > 0 | |||
|
|
|||
| def __array__(self, dtype=None): | |||
| def __array__(self, dtype=None, copy=None): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy argument of numpy.array is a boolean.
| def __array__(self, dtype=None, copy=None): | |
| def __array__(self, dtype=None, copy=False): |
| @@ -942,21 +931,26 @@ def set(self, params=None, **kwargs): | |||
| return | |||
|
|
|||
| if isinstance(params, (list, tuple)) and self.__len__() != len(params): | |||
| raise TypeError("status dict must be a dict, or a list of dicts of length {}".format(self.__len__())) | |||
| raise TypeError(f"status dict must be a dict, or a list of dicts of length {self.__len__()}") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise TypeError(f"status dict must be a dict, or a list of dicts of length {self.__len__()}") | |
| raise TypeError(f"Status dict must be a dict, or a list of dicts of length {self.__len__()}") |
| raise TypeError("expected parameter datum") | ||
| if not isinstance(datum, nestkernel.ParameterObject): | ||
| raise TypeError( | ||
| "expected low-level parameter object; use the 'CreateParameter()' function to create a 'Parameter'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "expected low-level parameter object; use the 'CreateParameter()' function to create a 'Parameter'." | |
| "Expected low-level parameter object; use the 'CreateParameter()' function to create a 'Parameter'." |
| msg = f"`{self._name}` is a read only kernel attribute." | ||
| raise AttributeError(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| msg = f"`{self._name}` is a read only kernel attribute." | |
| raise AttributeError(msg) | |
| raise AttributeError(f"`{self._name}` is a read only kernel attribute.") |
| @@ -80,7 +57,7 @@ def helpdesk(): | |||
|
|
|||
| """ | |||
|
|
|||
| docdir = sli_func("statusdict/prgdocdir ::") | |||
| docdir = nestkernel.ll_api_get_kernel_status()["docdir"] | |||
| help_fname = os.path.join(docdir, "html", "index.html") | |||
|
|
|||
| if not os.path.isfile(help_fname): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better consistence, please include msg content into FileNotFoundError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find it at line 64. Sorry for this comment, it was out of reach.
pnbabu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in the models directory look good to me overall. I have a few minor comments.
| @@ -261,12 +261,12 @@ class amat2_psc_exp : public ArchivingNode | |||
|
|
|||
| Parameters_(); //!< Sets default parameter values | |||
|
|
|||
| void get( DictionaryDatum& ) const; //!< Store current values in dictionary | |||
| void get( Dictionary& ) const; //!< Store current values in dictionary | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use Dictionary instead of dictionary in the comments here? I saw this in other models, but not here.
| void get( DictionaryDatum& ) const; //!< Store current values in dictionary | ||
| void set( const DictionaryDatum&, Node* node ); //!< Set values from dicitonary | ||
| void get( Dictionary& ) const; //!< Store current values in Dictionary | ||
| void set( const Dictionary&, Node* node ); //!< Set values from dicitonary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| void set( const Dictionary&, Node* node ); //!< Set values from dicitonary | |
| void set( const Dictionary&, Node* node ); //!< Set values from Dictionary |
| @@ -34,8 +34,7 @@ nest::Na::Na( double v_comp ) | |||
| // some default initialization | |||
| init_statevars( v_comp ); | |||
| } | |||
|
|
|||
| nest::Na::Na( double v_comp, const DictionaryDatum& channel_params ) | |||
| nest::Na::Na( double v_comp, const Dictionary& channel_params ) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| nest::Na::Na( double v_comp, const Dictionary& channel_params ) | |
| nest::Na::Na( double v_comp, const Dictionary& channel_params ) |
| @@ -326,15 +352,15 @@ correlation_detector::handles_test_event( SpikeEvent&, size_t receptor_type ) | |||
| } | |||
|
|
|||
| inline void | |||
| correlation_detector::get_status( DictionaryDatum& d ) const | |||
| nest::correlation_detector::get_status( Dictionary& d ) const | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need the nest namespace here when it's already in the namespace?
| { | ||
| device_.get_status( d ); | ||
| P_.get( d ); | ||
| S_.get( d ); | ||
| } | ||
|
|
||
| inline void | ||
| correlation_detector::set_status( const DictionaryDatum& d ) | ||
| nest::correlation_detector::set_status( const Dictionary& d ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
| // We must pass here if called by SetDefaults. In that case, the user will get and error | ||
| // message because the parameters for the synapse-specific optimizer have not been accessed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // We must pass here if called by SetDefaults. In that case, the user will get and error | |
| // message because the parameters for the synapse-specific optimizer have not been accessed. | |
| // We must pass here if called by SetDefaults. In that case, the user will get an error | |
| // message because the parameters for the synapse-specific optimizer have not been accessed. |
| * Compartment name list | ||
| * ---------------------------------------------------------------- */ | ||
|
|
||
| /* Harold Gutch reported some static destruction problems on OSX 10.4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep this comment?
| std::vector< std::string > ad; | ||
| for ( size_t j = 0; j < record_from_.size(); ++j ) | ||
| { | ||
| ad.push_back( LiteralDatum( record_from_[ j ] ) ); | ||
| ad.push_back( record_from_[ j ] ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we copy this to a new vector?
| /// std::vector< std::string > record_from; | ||
| /// | ||
| /// for ( size_t j = 0; j < record_from_.size(); ++j ) | ||
| /// { | ||
| /// record_from.push_back( record_from_[ j ] ); | ||
| /// } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment.
| { | ||
| if ( d->known( names::senders ) ) | ||
| auto get_or_create_nc = [ &d ]( NodeCollectionPTR& nc, const std::string& key ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be called just get_nc instead of get_or_create_nc, as an empty NC sis already created in the constructor?
Remove last instances of sli mentions from docs
jessica-mitchell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I will finalize the What's new page during the release process
A detailed description will follow. I create the PR now to allow references to it.
Review sections
doc/→ doc-teamexamples/→ doc-teambuild_support/ CI → @terhorstd, @gtrenschpackaging→ @terhorstd, @gtrenschbin→ @terhorstd, @gtrenschpynest→ @babsey, @otcathatsyamodels→ @poojanbabulibnestutil→ @JanVogelsang, @terhorstdnestkernelarchiving_node.cpp" –recordables_map.h` → @suku248kernel_manger.cpp" –vp_manager_impl.h` → @akorgorrecording_backend.cpp" –vp_manager_impl.h&archiving_node.cpp–io_manager_impl.h` → @JanVogelsang