-
Notifications
You must be signed in to change notification settings - Fork 71
Improve docstrings and documentation across optimization, GUI, and external modules #1152
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?
Improve docstrings and documentation across optimization, GUI, and external modules #1152
Conversation
|
|
||
| # The number of cores may need modifying depending on your current machine. | ||
| n_procs = 10 | ||
| n_procs = 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.
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.
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 would this change be necessary? Seems out of scope for this PR
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.
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] |
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.
More non-documentation code changes. These are probably safe, but would still need to be checked.
|
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. |
|
This is going to take a while to review, since it is a large number of changes. |
|
@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 @asoplata would it be better to have these submitted on a handful of files at a time? |
|
@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 | ||
| ) | ||
| """ |
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.
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.
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:
Scope
This PR fixes documentation-related issues for a large number of functions, but does not include documentation for all the files.