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

Fixing typos and alignments/spacings #61

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

cosimolupo
Copy link
Contributor

  • Fixes some typos here and there.
  • Adds or removes spaces and blank lines, where necessary for a more homogeneous formatting across similar files.

@cosimolupo cosimolupo requested review from rgutzen, mdenker and gulpgiulia and removed request for rgutzen February 15, 2024 11:50
Copy link
Collaborator

@rgutzen rgutzen left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning up the formatting and spelling in so many files. I just added a few comments where the fix is not too obvious.

@@ -102,6 +102,7 @@ use rule template as minima with:
'plot_channels', 'plot_tstart', 'plot_tstop',
img_name='minima_channel0.'+config.PLOT_FORMAT, config=config)


Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment headings are not preceded by two blank lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should instead use two blank lines before comment headings, so to better distinguish rules belonging to different "groups", given that rules from the same group are separated by double blank lines anyway. Let's decide a norm, and then let's stick to it.

cobrawap/pipeline/stage03_trigger_detection/Snakefile Outdated Show resolved Hide resolved
Comment on lines 65 to 64
/ f'{config.EVENT_NAME}_spatial_derivative.{config.PLOT_FORMAT}'
/ f'{config.EVENT_NAME}_spatial_derivative.{config.PLOT_FORMAT}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is purposefully under-indented to fit in a 79-character line bound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed.

STAGE_OUTPUT: 'channel-wise_measures.csv'
STAGE_OUTPUT: 'channel_wise_measures.csv'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be changed. We would need to evaluate if this is helpful or more consistent in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We previously changed the stage name, by using "channel_wave" instead of "channel-wave". So I was acting here coherently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The naming convention for the stage names requires no use of '-' since it can cause issues when chaining together the pipeline from the stages. However, this is separate from conventions for file names. I think with the current naming conventions, this change in file names wouldn't make it more consistent. However, we could think about standardizing the naming of stages and their output files some more, also for the other stages.
For the stage05 this may entail also adapting the stage names. One possibility would be, for example, stage05_wave_wise_characterization :-> wave_wise_characterization.csv and stage05_channel_wise_characaterization :-> channel_wise_characterization.csv.
Such changes may, however, influence existing pipeline applications. Therefore, I would postpone such improvements of the naming conventions to version 0.2.

STAGE_OUTPUT: 'wave-wise_measures.csv'
STAGE_OUTPUT: 'wave_wise_measures.csv'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be changed. We would need to evaluate if this is helpful or more consistent in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We previously changed the stage name, by using "channel_wave" instead of "channel-wave". So I was acting here coherently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

see comment above

cobrawap/pipeline/utils/Snakefile Outdated Show resolved Hide resolved
cobrawap/pipeline/utils/neo_utils.py Show resolved Hide resolved
@cosimolupo cosimolupo requested a review from rgutzen February 21, 2024 13:23
CLI.add_argument("--data", nargs='?', type=str,
help="path to input data in neo format")
CLI.add_argument("--output_dir", nargs='?', type=str,
help="path to output directory")
CLI.add_argument("--img_name", nargs='?', type=str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

very very minor, just to be consistent with other changes introduced above, assume that each argument line has a "help" option

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I inserted a suggestion, with the "empty" help option in a new line, to be coherent with what done above, but for short lines can also be put in the same line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right. I could add help="" where still missing, so to have it for every single command-line arg.

CLI.add_argument("--data", nargs='?', type=str,
help="path to input data in neo format")
CLI.add_argument("--output_dir", nargs='?', type=str,
help="path to output directory")
CLI.add_argument("--img_name", nargs='?', type=str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
CLI.add_argument("--img_name", nargs='?', type=str)
CLI.add_argument("--img_name", nargs='?', type=str)
help="")

CLI.add_argument("--img_name", nargs='?', type=str)
CLI.add_argument("--time_window", nargs='?', type=float, default=0.4,
help="size of the plotted window in seconds.")
help="size of the plotted window in seconds.")
CLI.add_argument("--colormap", nargs='?', type=str, default='viridis')
Copy link
Collaborator

Choose a reason for hiding this comment

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

"help" also here, but maybe not needed to be present each time...?

@mdenker mdenker assigned mdenker and unassigned mdenker Feb 22, 2024
Copy link
Collaborator

@rgutzen rgutzen left a comment

Choose a reason for hiding this comment

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

Looks all fine now.
I just added a comment to postpone the changes to file naming conventions to a later separate PR.

@cosimolupo
Copy link
Contributor Author

Ok then, I will provide a new commit right now, following @rgutzen's suggestion of postponing changes in naming conventions to a future PR.

@rgutzen rgutzen added the formatting improvement of code style and structure label Feb 28, 2024
Postponing this change to a future PR
@mdenker
Copy link
Member

mdenker commented Mar 7, 2024

New issue #63 is used to collect ideas for naming conventions.

@cosimolupo cosimolupo self-assigned this Mar 20, 2024
Copy link
Member

@mdenker mdenker left a comment

Choose a reason for hiding this comment

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

I could not spot any remaining issues -- all good from my side.

@mdenker mdenker merged commit a1c0579 into NeuralEnsemble:master Mar 22, 2024
cosimolupo added a commit to APE-group/cobrawap that referenced this pull request Mar 22, 2024
@cosimolupo cosimolupo deleted the bugfix/typos branch June 21, 2024 13:35
mdenker pushed a commit that referenced this pull request Jul 18, 2024
* Updates the PR according to the presence of robust_t novel function in neo_utils

* Adding "None" as a permitted value in all plotting scripts

* Minor change in config template files

* Fix for extra spacings added when merging PR #61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatting improvement of code style and structure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants