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

Update activity score calculation algorithm #341

Merged
merged 4 commits into from
Oct 4, 2021
Merged

Update activity score calculation algorithm #341

merged 4 commits into from
Oct 4, 2021

Conversation

3cpt
Copy link
Contributor

@3cpt 3cpt commented Aug 13, 2021

This PR contains changes to the Repository Activity Score algorithm.

  • replace watchers_count by subscribers_count - because stars are the same as watchers, so I just kept stargazers and replaced watchers by subscribers (read more about this here).
  • fix slice number - I'm not a JS pro, but seems that we just want to slice the 13 (~3 months)
  • simplify check - some validations can be made by just add the ?
  • repo JSON access standardization - JS versatility allow using json['prop'] or json.prop. I picked one for better reading.

* replace `watchers_count` by `subscribers_count`
* fix slice number
* simplify check
* repo JSON access standardization
@spier
Copy link
Member

spier commented Aug 14, 2021

Thanks for the contribution @3cpt!

Very cool that you are reviewing the pseudo code here.
I am not even sure if it was meant to represent JS but you are probably right.

You mentioned elsewhere that you are planning to add a score calculation function to one of the reference implementations of the crawlers, right?

Great catch with the watchers/stargazers/subscribers topic. I need to learn more about this. I am a bit confused after reading the article that you referenced above. Maybe I just got to read it again. :)

I left one inline comment for you, to see if we could improve readability of the score calculation even further.

Not sure how we could go about validating the pseudo code here.

@Michadelic you will find this one interesting. Any feedback and ideas what we could use to validation the algorithm?

Co-authored-by: Sebastian Spier <github@spier.hu>
@3cpt
Copy link
Contributor Author

3cpt commented Aug 16, 2021

You mentioned elsewhere that you are planning to add a score calculation function to one of the reference implementations of the crawlers, right?

Yep. That's why I start analyzing the algorithm, I'm implementing a Python version to integrate in the zkoppert/innersource-crawler. Should we add a section in the pattern to implementations of the algorithm?

Great catch with the watchers/stargazers/subscribers topic. I need to learn more about this. I am a bit confused after reading the article that you referenced above. Maybe I just got to read it again. :)

As far as I understood, they changed watchers to stars and that created a small problem (More and Other)

Not sure how we could go about validating the pseudo code here.

Agree. What about use the mock data from the planet's repo.json and generate the score instead of mock it too?

@spier
Copy link
Member

spier commented Aug 24, 2021

@3cpt just confirming that I didn't forget about your PR. Would just like to wait for @Michadelic for feedback.
(it might still be holiday season in some places so let's give him some time :)).

@3cpt
Copy link
Contributor Author

3cpt commented Aug 25, 2021

@spier No worries. I replicated the same code in Python and C# to validate the consistency of the numbers.
So if someone uses the algorithm to gather the score, please, share it! Real data is always good in this case.

@Michadelic
Copy link
Contributor

Hi @3cpt @spier, indeed there is holiday season over here so please excuse my late response.
Thank you very much for your great findings, will take a look in detail now and provide feedback in the code directly.

I took this code directly from our running crawler on GitHub enterprise, so it is a working JavaScript implemention, but for sure it can be further tweaked and adjusted. If you spot more issues just let me know.

Michael

Copy link
Contributor

@Michadelic Michadelic left a comment

Choose a reason for hiding this comment

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

Great simplifications, just the slice change should be reverted (see my comment)

patterns/2-structured/repository-activity-score.md Outdated Show resolved Hide resolved
@spier
Copy link
Member

spier commented Aug 26, 2021

@Michadelic would you mind reviewing/approving this PR once it is in a state that is ready to be merged?
I don't feel knowledgeable enough myself about the activity score to do this myself. Cheers.

@3cpt
Copy link
Contributor Author

3cpt commented Aug 30, 2021

During my last check I verified that the + 1 in the L64 and L70 (in the previous version) lead the result of the multiplier to > 1, when the rule in the comment says that should be between 0...1.
So, if, in the worst case scenario we get 0, 0 / x = 0, if it is x / x = 1.

Please, your feedback is welcome, and I could be doing my math wrongly.

@spier spier changed the title update score calculation algorithm Update activity score calculation algorithm Sep 2, 2021
@spier spier added the 📖 Type - Content Work Working on contents is the main focus of this issue / PR label Sep 10, 2021
Copy link
Contributor

@Michadelic Michadelic left a comment

Choose a reason for hiding this comment

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

please keep the "1+" as explained in the comment, other changes for readability are appreciated 👍

patterns/2-structured/repository-activity-score.md Outdated Show resolved Hide resolved
@spier
Copy link
Member

spier commented Sep 29, 2021

@Michadelic would you mind reviewing the latest status of this PR again, and then approving the PR when everything looks good?

To me it looks like @3cpt implemented the change that you asked for (thanks for that!) for but I am not 100% sure so I figured I rather ask you for confirmation.

Copy link
Contributor

@Michadelic Michadelic 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 the improvements, looks good now!

@Michadelic
Copy link
Contributor

@spier PR can be merged now, looking good

@spier spier merged commit 677dd89 into InnerSourceCommons:main Oct 4, 2021
@spier
Copy link
Member

spier commented Oct 4, 2021

Thank you for this contribution @3cpt !!! Much appreciated fix.

Also we are looking forward to see your reference implementation.
Once that is live, then we could also link to that from this pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 Type - Content Work Working on contents is the main focus of this issue / PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants