Skip to content

Comments

Automatically detect the profile format.#1

Open
jarusified wants to merge 10 commits intodevelopfrom
detect-profile-format
Open

Automatically detect the profile format.#1
jarusified wants to merge 10 commits intodevelopfrom
detect-profile-format

Conversation

@jarusified
Copy link
Owner

No description provided.

@jarusified jarusified changed the title Push the initial structure for code Automatically detect the profile format. Jan 22, 2021
Copy link
Collaborator

@bhatiaharsh bhatiaharsh left a comment

Choose a reason for hiding this comment

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

looks good overall

self.inc_metrics = [] if inc_metrics is None else inc_metrics

@staticmethod
def from_path(dirname_or_data, query, profile_format=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

query should have a default to empty string

query (str):
profile_format (str): Override for the profile_format, if detection
of format fails.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

assertions for input types

'lists': GraphFrame.from_lists(dirname_or_data),
}

if profile_format:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if profile_format is not None

note that your condition will work, but i dont like using it. by explicitly writing is not None you tell the rreader what you want and also catch the case wherre i pass profilee_format=True (a bool)


profile_format = GraphFrame._detect_profile_format(dirname_or_data)

if query:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if len(query) > 0

}

# Determine dirname_or_data is a str and a path exists
if type(dirname_or_data) == 'str' and os.path.exists(os.path.dirname(dirname_or_data)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure what the purpose of the secnd condition is



# check if it is a file
elif os.path.isfile(dirname_or_data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

in principle, you dnt need elif.. just if

return 'pstats'
elif _file_ext == 'json':
# TODO: Check if we can just load the key and dtype of JSON.
# We could also be unnecessarily read the data again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

good comment. agreed. ask abhinav how he wants to handle this

return 'timemory'
elif _file_ext == "dot":
return 'grpof'
elif type(dirname_or_data) == 'list':
Copy link
Collaborator

Choose a reason for hiding this comment

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

i generally like to put simpler case at thee top.. thiis function wont be call many times so there is no performance hit. otherwise, you'd want to put more frequent cases first.

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.

2 participants