Skip to content

Conversation

@sketch123456
Copy link

This PR improves docstrings across several modules in HNN-Core, focusing on optimization, GUI, external interfaces, and examples. The updates aim to increase clarity, ensure consistency, and make the codebase more accessible to new contributors and users.

Changes Made:

  • Standardized docstrings to follow a consistent style (NumPy-style parameter/returns sections).
  • Clarified function purposes, parameter roles, and return values.
  • Expanded documentation for optimization routines (e.g., evoked optimization, general optimization, Bayesian optimization).
  • Improved docstrings for GUI modules to clarify responsibilities and improve readability.
  • Added context and explanations in example scripts.

Scope

  • Documentation-only changes.
  • No functional code modifications.
  • All existing tests should pass without changes.

This PR fixes documentation-related issues for a large number of functions, but does not include documentation for all the files.


# The number of cores may need modifying depending on your current machine.
n_procs = 10
n_procs = 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: despite the PR description, this line does change actual code. This means that there are other places that this PR could have changed actual code instead of documentation, and therefore this PR would need to be inspected very carefully for accidental code changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would this change be necessary? Seems out of scope for this PR

Copy link
Author

Choose a reason for hiding this comment

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

This change was a mistake, resulting from testing example scripts on my local machine without reverting the changes.

)

tagert_names = simulation_names[:-1]
target_names = simulation_names[:-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

More non-documentation code changes. These are probably safe, but would still need to be checked.

@sketch123456
Copy link
Author

I added an additional commit providing documentation for functions in 'check.py.' As discussed, this PR only modifies documentation for the 3 functions in this file and does not make any code edits.

@asoplata
Copy link
Collaborator

This is going to take a while to review, since it is a large number of changes.

@ntolley
Copy link
Collaborator

ntolley commented Oct 21, 2025

@sketch123456 glad to see you submitting PR's!

As a quick comment, we tend to avoid large sweeping changes in PR's

In general it's best to break up large updates/refactors into multiple focused PRs so that 1) you're not interrupting other people's code with merge conflicts, 2) the reviewers can actually understand the code and properly test it, and 3) the main branch may update during the extended review of the PR meaning you need to rebase it (merge conflicts work both ways)

@asoplata would it be better to have these submitted on a handful of files at a time?

@asoplata
Copy link
Collaborator

@ntolley I think keeping it in one PR (and not making new ones) makes sense in this case. The vast majority of the changes are useful here and worth keeping, after a tiny bit of polish and double-checking that they're correct. In general I prefer to stick with existing PRs instead of cancelling them and making new ones, but that's just a small personal preference. A single PR also keeps most of the conversation in the same place, which can make things easier to understand for new developers.

return check_sim_plot_types(
new_sim_name.new, plot_type_selection, target_data_selection, data
)
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

When the docstring was added here, the actual body of the function was deleted. This is now an empty function with only a docstring and no actual code. Reminder to self that need to keep an eye out for other code deletions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants