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

Fix missing colon and correct comment #2525

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

JanVogelsang
Copy link
Contributor

@JanVogelsang JanVogelsang commented Nov 14, 2022

There was a missing colon in models/iaf_cond_alpha_mc.h which caused a reference to not be displayed correctly.

A comment in nestkernel/connection.h stated incorrect bit-sizes, probably an artifact of older versions.

@JanVogelsang JanVogelsang marked this pull request as ready for review November 14, 2022 11:19
@jessica-mitchell jessica-mitchell added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 14, 2022
@jessica-mitchell jessica-mitchell added this to In progress in Documentation via automation Nov 14, 2022
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@JanVogelsang I can only comment on the change to the iaf_cond_alpha.h, and that looks good.

Someone who knows the C++ should check the other line @jougs @heplesser ?

@heplesser
Copy link
Contributor

@JanVogelsang Thank you for this PR. I will merge it now but would like to ask you to further improve the doxygen comments on data members in Connection:

/* the order of the members below is critical
as it influcences the size of the object. Please leave unchanged
as
targetidentifierT target_;
SynIdDelay syn_id_delay_; //!< syn_id (char) and delay (24 bit) in
timesteps of this
connection
*/
targetidentifierT target_;
//! syn_id (char) and delay (24 bit) in timesteps of this connection
SynIdDelay syn_id_delay_;

The comment on syn_id_delay_ is partly redundant with the non-doxygen comment before target_. Could comments on the two members be combined into a doxygen group? I also think that it is not sensible to give field sizes in the comments, given that depend on compiler flags and could change over time, see

constexpr uint8_t NUM_BITS_SYN_ID = 9U;

and

constexpr uint8_t NUM_BITS_SYN_ID = 6U;

@heplesser heplesser merged commit 93dc0f9 into nest:master Nov 14, 2022
Documentation automation moved this from In progress to Done Nov 14, 2022
@JanVogelsang JanVogelsang deleted the comment-fixes branch November 15, 2022 08:02
@JanVogelsang
Copy link
Contributor Author

Fair point, I will look into this.

Didn't know the bitmasks were different for HPC and non-HPC, is that noted in the user documentation somewhere as well?
A user should definitely know that a model they run on their local system is not guaranteed to run on an HPC system as well, which is a questionable decision anyway. Shouldn't the typical workflow be to test a model on your local system with lower scale and then run the same model on a cluster by just scaling up the network size? And wouldn't you usually use even more different synapse types on an HPC system than you would on a lower scale one? I understand that the maximum number of threads or ranks will be limited on a non-HPC system, therefore reducing the size of the bitmasks won't hurt, but reducing the number of different synapse types might actually hurt.
Although I understand, that there is no other choice basically and I should probably see it the other way around. The number of different synapse types is not reduced on HPC systems, it is instead increased on non-HPC ones, because why not, there are free bits to spend anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Documentation
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants