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

Make usage of vintage_and_active_years() in the tutorials consistent #900

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Dec 12, 2024

Closes #897 and #899:

  • For some tutorials, remove year_df entirely because it is unused. For others, make its usage consistent with westeros_baseline.
  • Address a FutureWarning from pandas.

How to review

  • Read the diff and note that the CI checks all pass.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • [ ] Add, expand, or update documentation. Just fixing tutorials.
  • [ ] Update release notes. Just fixing tutorials.

@glatterf42 glatterf42 added the tutorials Westeros and Austrian tutorials label Dec 12, 2024
@glatterf42 glatterf42 self-assigned this Dec 12, 2024
@glatterf42 glatterf42 linked an issue Dec 12, 2024 that may be closed by this pull request
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.6%. Comparing base (ab9b707) to head (0109580).
Report is 6 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #900   +/-   ##
=====================================
  Coverage   95.6%   95.6%           
=====================================
  Files         46      46           
  Lines       4338    4338           
=====================================
  Hits        4148    4148           
  Misses       190     190           

@glatterf42 glatterf42 force-pushed the fix/tutorials-vintage-and-active-years branch 2 times, most recently from 1884324 to 76bdfc6 Compare January 7, 2025 13:15
@glatterf42 glatterf42 requested a review from khaeru January 7, 2025 13:40
@khaeru khaeru force-pushed the fix/tutorials-vintage-and-active-years branch from 76bdfc6 to 0109580 Compare January 7, 2025 14:39
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

Looks great.

In the future when we receive/review changes/additions to the tutorials, we can apply the same principle: in general, defining variables and then not later using them isn't desirable. It's especially bad in instructional material, because it interferes with the learner's process of forming a mental model of how the code works/should be used. For instance, if we say "Suppose A, B, and C. Combining B and C, we get…" —the reader is forced to keep part of their attention on "A was mentioned, I need to keep listening to understand what it's for."

@khaeru khaeru merged commit a60715a into main Jan 7, 2025
27 checks passed
@khaeru khaeru deleted the fix/tutorials-vintage-and-active-years branch January 7, 2025 15:45
@khaeru khaeru added this to the 3.10 milestone Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tutorials Westeros and Austrian tutorials
Projects
None yet
2 participants