Skip to content

review add#1

Open
mtorabi59 wants to merge 1 commit intoemptyfrom
review_
Open

review add#1
mtorabi59 wants to merge 1 commit intoemptyfrom
review_

Conversation

@mtorabi59
Copy link
Copy Markdown
Owner

No description provided.

@@ -0,0 +1,4 @@
# dFC
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

more details needed. For example, you can explain each method, or what steps should be done to run your own data etc.

Copy link
Copy Markdown

@jeromedockes jeromedockes left a comment

Choose a reason for hiding this comment

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

first batch of small comments, mostly details for now

@@ -0,0 +1,149 @@
from functions.dFC_funcs import *
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

in general avoid import * as it pollutes the namespace and makes it hard to know where names come from.
linting tools will complain about it; it's a good idea to use them -- have a look for example at pylint and flake8

@@ -0,0 +1,3472 @@
#!/usr/bin/env python3
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if this is a module meant to be imported you don't need a shebang

@@ -0,0 +1,3472 @@
#!/usr/bin/env python3
# -*- coding: utf-8 -*-
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

utf-8 is the default: https://peps.python.org/pep-3120/

"""
Created on Sun Jun 13 22:34:49 2021

@author: mte
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

?

import networkx as nx
from scipy.spatial import distance
from joblib import Parallel, delayed
import os
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

put standard lib imports before other imports (see pep8)


###### DATA PARAMETERS ######

output_root = './../../../../../RESULTs/methods_implementation/'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • consider putting paths in env variables or a config files rather than source files
  • use pathlib to manipulate filesystem paths


# DATA_type is either 'sample' or 'Gordon' or 'simulated' or 'ICA'
params_data_load = { \
'DATA_type': 'Gordon', \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you don't need these backticks


@author: mte
"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you could add a setup.py to make it possible to install the package so you don't have to manipulate the python path in your notebook

Clustering_pear_corr
'''

if name_lst_key is None:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

try to keep function short; they should at least fit on a screen ie less than ~40 lines (there may be rare exceptions), and in most cases be even much shorter than that (~5-10 lines)

else:
plt.show()

'''
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is this to comment out code?
trust version control and delete the code you don't need instead of commenting it out

@@ -0,0 +1,360 @@

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what testing framework do you use?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants