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

Improve the editor's load plots #43

Merged
merged 6 commits into from
Dec 20, 2024

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Aug 27, 2024

Apply changes suggested in #24.

  • Fix import of custom function and use purple color
  • Remove ignored editors upon creation
  • Use shared axis to make them all in same scale
  • Put editor's name in the right hand side of the plot
  • Don't show plots for lwasser and xmnlab to bring the min date to a more recent date

Fix the import of the `count_edits_by_quarter` function: now it's being
imported from the `pyosmeta` package. Use purple color for the bars in
the chart.
Use list comprehension to creating the `editors` list and removing the
ignored ones at the time of creation. Remove the unnecessary check for
ignored editors in the charts for loop.
Avoid the need to set the time and number of edits domains to set the
same scale for every chart by using the `resolve_scale` method.
@lwasser
Copy link
Member

lwasser commented Aug 27, 2024

Thank you for this, @santisoler!!! I will pause my current pr #44 to make sure I don't interfere with your work again 🙈 . I'm trying to reorganize this repo to make space for our annual report and to add a page on contributor data. I haven't touched this notebook, however, so it should be easy enough to merge. Please just let me know when you'd like me to review it again.

I think I'll undo moving this notebook in that pr to ensure we can merge both, and then I'll implement a final pr that moves everything around for the website!

@santisoler
Copy link
Member Author

Hi @lwasser! Don't worry about me. Please, keep working on #44. I don't think I'll have too many problems updating this branch. Besides, I don't want your PR to be stalled because of mine.

I plan to work on the notebook only, so as long as you don't expect to make a lot of changes to it, it should be fine.

@lwasser
Copy link
Member

lwasser commented Aug 27, 2024

Okay, that's wonderful. @santisoler, yes, I don't plan to change the notebook you are working on. I'm working on a contributor notebook and reorganized the repo for a cleaner repository structure matching the website version. I appreciate your patience!! 🙌🏻

I only saw your new pr when I opened mine. Thank you again for your work here!!

@lwasser
Copy link
Member

lwasser commented Aug 28, 2024

@santisoler i made two tiny changes to the current-reviews-data notebook to make CI happy.

You can make them here in your branch and overwrite the main as you wish, as there are no code changes on main.

  1. I modified the license to be BSD-3-Clause rather than BSD-3. myst build catches that and was throwing warnings
  2. I imported your functions module into our helper package. This allows us to install the modules into an envt and create subfolders and organize things.

from pyosmetrics.functions import count_edits_by_quarter

Please feel free to completely overwrite main with this pr if you rebase / or copy over !! As i made no real code changes - just fixes to make CI happy. ✨

no rush at all on this i just wanted you to know so it's not a surprise when you see a conflict here.

@willingc willingc added the draft label Nov 14, 2024
Ignore lwasser and xmnlab editors to bring the min date to a recent
date. Locate the name of the editors to the right of each one of the
plots.
@santisoler
Copy link
Member Author

@lwasser after a few months I managed to update this PR.

I applied a few more changes (check out the PR description) following the ideas you mentioned in #24.

I tried to set the step for the vertical axes to 1.0 through the tickMinStep parameter like this:

charts = [
    alt.Chart(edits.loc[edits.editor == editor])
    .mark_bar(color="purple")
    .encode(
        x=alt.X("yearquarter(Date):T"),
        y=alt.Y("count(package_name)", title="Number of edits per quarter").axis(tickCount=1.0),
        tooltip=["yearquarter(Date)", "count(package_name)"],
    )
    .properties(
        title=alt.TitleParams(text=f"{editor}", fontSize=18, orient="right", angle=0, align="right"),
        width=600,
        height=200,
    )
    for editor in editors
]

But it doesn't seem to reflect the change we want. I'm not sure if there's something wrong I'm doing of this is a bug in Altair.

@santisoler santisoler marked this pull request as ready for review December 19, 2024 21:26
@lwasser
Copy link
Member

lwasser commented Dec 20, 2024

hey there @santisoler no worries on the ticks. i found working with altair to be a bit clunky when it came to refining plots!! How about we merge this as is. And we can always refine things later (if we want to!). Does that sound ok to you?

@lwasser lwasser removed the draft label Dec 20, 2024
@santisoler
Copy link
Member Author

I totally agree. Let's merge this one, we can keep improving it!

Feel free to open an Issue to list the things you would like to see changed.

@lwasser
Copy link
Member

lwasser commented Dec 20, 2024

Awesome, @santisoler I'll merge it now. I REALLy appreciate all of the work that you did here. Thank you for the contributions, and we can definitely work on refining things in 2025.

I'm not sure if you saw this anywhere so i'll mention it here. pyOS is taking a break through the new year 🚀 I hope that you and your family have a wonderful new year!! ✨

@lwasser lwasser merged commit 461901c into pyOpenSci:main Dec 20, 2024
2 checks passed
@santisoler santisoler deleted the improve-editors-plots branch December 20, 2024 23:17
@santisoler
Copy link
Member Author

Thanks @lwasser! It's a pleasure to contribute to pyOS 😊.

I'm not sure if you saw this anywhere so i'll mention it here. pyOS is taking a break through the new year 🚀 I hope that you and your family have a wonderful new year!! ✨

That sounds great! I'm planning to rest during the holidays as well. Wish you wonderful holidays and a great new year!

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