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.

Sign up for GitHub

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

Jump to bottom

[graal] Add support of Graal's CoCom Backend to ELK [File-level + Study] (WIP) #664

Closed
wants to merge 2 commits into from
Closed

[graal] Add support of Graal's CoCom Backend to ELK [File-level + Study] (WIP) #664

wants to merge 2 commits into from

Conversation

inishchith
Copy link
Contributor

@inishchith inishchith commented Jul 19, 2019 •
edited
Loading

Address: inishchith/gsoc#8

TODO:

  • Add tests
  • Update docstrings
  • Update commit description

NOTE:

  • The failing test is due to Graal module not being a part of requirements.txt(dependencies) and is a part of this PR just to check the coverage of new additions.

Signed-off-by: inishchith inishchith@gmail.com

@inishchith
[graal] Add support of Graal's CoCom Backend to ELK
30ea904 
Add support of Graal's CoCom backend to ELK and it's corresponding tests

Signed-off-by: inishchith <inishchith@gmail.com>
@inishchith inishchith changed the title [graal] Add support of Graal's CoCom Backend to ELK [graal] Add support of Graal's CoCom Backend to ELK (WIP) Jul 19, 2019
@inishchith inishchith force-pushed the gsoc-graal-2019-cocom+study-2.0 branch from c44f4d4 to 96425ce Compare July 19, 2019 17:01
@coveralls
Copy link

coveralls commented Jul 19, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 1566

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 52 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 68.012%
Files with Coverage Reduction New Missed Lines %
/home/travis/build/chaoss/grimoirelab-elk/grimoire_elk/utils.py 52 60.56%
Totals Coverage Status
Change from base Build 1564: -0.2%
Covered Lines: 5747
Relevant Lines: 8450
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1547

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 52 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 68.063%
Files with Coverage Reduction New Missed Lines %
/home/travis/build/chaoss/grimoirelab-elk/grimoire_elk/utils.py 52 60.56%
Totals Coverage Status
Change from base Build 1545: -0.2%
Covered Lines: 5735
Relevant Lines: 8426
💛 - Coveralls

@inishchith inishchith changed the title [graal] Add support of Graal's CoCom Backend to ELK (WIP) [graal] Add support of Graal's CoCom Backend to ELK [File-level + Study] (WIP) Jul 23, 2019
@inishchith inishchith mentioned this pull request Jul 23, 2019
Closed
3 tasks
@valeriocos valeriocos self-requested a review July 23, 2019 11:49
valeriocos
valeriocos reviewed Jul 23, 2019
View reviewed changes
Copy link
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

The comments are similar to: #669 (review). In a nutshell, the approach looks good, however the study code in the cocom and colic enrichers seems really similar. It could be nice to abstract this logic in a new class (contained in a dedicated file) together with the queries. WDTY?

Thanks

@inishchith inishchith force-pushed the gsoc-graal-2019-cocom+study-2.0 branch from 96425ce to 82c368a Compare July 25, 2019 16:46
@inishchith
Copy link
Contributor Author

inishchith commented Jul 25, 2019 •
edited
Loading

Tasks

  • New implementation of Study Enricher
  • Add fields ( Lines per comment, Lines per blank)
  • Work on issue addressed @ inishchith@12ec7bf#r34368615

+ the first comment tasks

@inishchith inishchith force-pushed the gsoc-graal-2019-cocom+study-2.0 branch 5 times, most recently from cb01d31 to 2555cf1 Compare July 26, 2019 17:22
valeriocos
valeriocos reviewed Jul 26, 2019
View reviewed changes
Copy link
Member

@valeriocos valeriocos 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 @inishchith for the PR. It looks great, however some minor changes should be added.
I left some comments, please let me know what you think.

grimoire_elk/enriched/cocom.py
def __add_derived_metrics(self, file_analysis, eitem):
""" Add derived metrics fields """

# TODO: Fix Logic: None rather than 1
Copy link
Member

Choose a reason for hiding this comment

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

Generally the proportion is calculated based on LOC, thus the attributes should be funs/LOC, comments/LOC blanks/LOC. The code could be refactored as follows, WDYT?

loc = eitem.get('loc', None)
comments = eitem.get('comments', None)
functions = eitem.get('num_funs', None)

eitem["comment_lines_per_loc"] = None
eitem["blank_lines_per_loc"] = None
eitem["functions_per_loc"] = None

if loc:
       eitem["comment_lines_per_loc"] = comments / loc if comments else None
       eitem["blank_lines_per_loc"] = blanks / loc if blanks else None
       eitem["functions_per_loc"] = functions / loc if functions else None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valeriocos I wanted to discuss this and get more clarity. ( the changes shouldn't take time so).

Generally the proportion is calculated based on LOC, thus the attributes should be funs/LOC, comments/LOC blanks/LOC.

  • I had thought of adding loc_per_(metric) as for instance, it's easier to understand if we say there are X lines of code per function ( which means in the corresponding file, each function has around X lines of code). Similarly for comments and blanks.
  • Whereas the otherwise functions_per_loc would give us a result (in most of the case) less than 1 for instance, 0.34 functions per line of code. Would it make sense to have this information or the former? (Please correct me if I misunderstood something here 🙈 )

Let me know what you think :)

Copy link
Member

@valeriocos valeriocos Jul 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's better to have what you propose, each function has around X lines of code looks much more interesting as info. However, for comments and blank lines knowing each comment/blank lines has around X lines of code sounds a bit strange, while 0.34 comments/blank lines per line of code seems better. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valeriocos Thanks for the quick response :).
We can discuss more on this in the next meeting with @aswanipranjal and @jgbarah, and make necessary changes ( I'll keep this as unresolved until then )

Copy link
Member

Choose a reason for hiding this comment

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

Sure! I won't be in the next meeting (vacations)

Copy link
Contributor Author

@inishchith inishchith Jul 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@valeriocos Sorry for the delayed response.

Sure! I won't be in the next meeting (vacations)

Oh Okay!. As of now, I've made the changes. Thanks!

grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
grimoire_elk/enriched/cocom.py
evolution_item["total_" + metric] = evolution_item.get("total_" + metric, 0) + file_details[metric]

# TODO: Fix Logic: None rather than 1
evolution_item["total_loc_per_comment_lines"] = evolution_item["total_loc"] / \
Copy link
Member

Choose a reason for hiding this comment

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

I would mimic the logic described in the comment of __add_derived_metrics, WDYT?

grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
grimoire_elk/enriched/cocom.py Outdated Show resolved Hide resolved
@inishchith inishchith force-pushed the gsoc-graal-2019-cocom+study-2.0 branch from 2555cf1 to fa0fbad Compare July 27, 2019 05:56
@inishchith
[study] Add repository analysis study
2cac375 
Signed-off-by: inishchith <inishchith@gmail.com>
@inishchith inishchith force-pushed the gsoc-graal-2019-cocom+study-2.0 branch from fa0fbad to 2cac375 Compare July 27, 2019 07:44
@inishchith inishchith mentioned this pull request Jul 28, 2019
Closed
3 tasks
valeriocos
valeriocos reviewed Jul 28, 2019
View reviewed changes
Copy link
Member

@valeriocos valeriocos 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 @inishchith . The PR looks perfect, I left a minor comment.

grimoire_elk/enriched/cocom.py Show resolved Hide resolved
@inishchith inishchith mentioned this pull request Jul 29, 2019
Merged
8 tasks
@inishchith
Copy link
Contributor Author

closing in reference to #672

@inishchith inishchith closed this Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valeriocos valeriocos valeriocos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@inishchith @coveralls @valeriocos