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

Housekeeping for 2024-W13 #809

Merged
merged 12 commits into from
Mar 29, 2024
Merged

Housekeeping for 2024-W13 #809

merged 12 commits into from
Mar 29, 2024

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Mar 28, 2024

  • Adapt typing to genno 1.25.
  • Defer import of pyam until needed; speeds up import of message_ix.
  • Improve documentation cross-references using sphinx extensions provided by genno 1.26.
  • Adjust code to pass type checks with pandas-stubs.
  • Reduce max-complexity 15 → 13.

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.
  • Update release notes. No change in functionality.

@khaeru khaeru self-assigned this Mar 28, 2024
@khaeru khaeru added enh New features & functionality ci Continuous integration labels Mar 28, 2024
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.4%. Comparing base (e33ae53) to head (f4cbe57).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #809   +/-   ##
=====================================
  Coverage   95.4%   95.4%           
=====================================
  Files         46      46           
  Lines       4351    4354    +3     
=====================================
+ Hits        4153    4156    +3     
  Misses       198     198           
Files Coverage Δ
message_ix/core.py 97.7% <100.0%> (+<0.1%) ⬆️
message_ix/macro.py 96.7% <100.0%> (ø)
message_ix/report/operator.py 97.2% <100.0%> (+<0.1%) ⬆️
message_ix/report/pyam.py 100.0% <100.0%> (ø)
message_ix/testing/__init__.py 99.6% <100.0%> (-0.1%) ⬇️
message_ix/tests/test_feature_duration_time.py 100.0% <100.0%> (ø)
message_ix/tools/add_year/__init__.py 66.2% <100.0%> (ø)
message_ix/util/__init__.py 98.7% <ø> (ø)

Copy link
Member

@glatterf42 glatterf42 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 to me, but one question: in the output of the docs-build, there are numerous warnings referring to genno.core docstrings. Several methods and documents could not be found. They were not present in the last run of latest, though some errors from there are fixed here. Do you want to fix them here or should this be a new PR?

@khaeru
Copy link
Member Author

khaeru commented Mar 29, 2024

Do you want to fix them here or should this be a new PR?

I think these should perhaps be PRs upstream. This is basically what we tried to solve in khaeru/genno#94 and khaeru/genno#108, and I now have a different idea of how to resolve these issues.

Previously (those above PRs) we tried to use the intersphinx_mapping to force lookups to resolve correctly. I don't know if this does/can work.

Instead, I think we can change some references like this:
https://github.com/iiasa/ixmp/blob/5713fc14c0206c664405e4dd59576808b65832b9/ixmp/core/timeseries.py#L326
to this:

:meth:`.Platform.add_timeslice`

Why should that hopefully work?

  1. Within ixmp's docs:
    a. ".add_timeslice" as the reference target, with the prefixing ".", means "a method named add_timeslice, anywhere within this Sphinx project". This is resolved with "ixmp.Platform.add_timeslice", because that's the only internal reference target that (a) is a method and (b) has that name. In other words, the "." prefix does not trigger that matching behaviour inclusive of intersphinx inventories or external targets. I think there's no easy way to force intersphinx to do so.
    b. ".Platform.add_timeslice" resolves to the same target but is more verbose, so often we write (a).
  2. Within message_ix's docs, if the docstring for ixmp.TimeSeries.add_timeseries is shown as part of the subclass message_ix.Scenario:
    a. ".add_timeslice" as the reference target has the same meaning. But given the "." prefix, Sphinx does not find any (a) method (b) named "add_timelice" within the same project, so the reference is unresolved and we see a warning.
    b. ".Platform.add_timeslice" as the target together with the new genno.compat.sphinx.rewrite_refs extension, gets rewritten to "ixmp.Platform.add_timeslice", and this full ref is found exactly in the intersphinx inventory.

This will be a little awkward because we basically will need to:

  • Adjust the internal refs in genno that are causing warnings in ixmp and/or message_ix.
  • Adjust the internal refs in ixmp that are causing warnings in message_ix.

…however, I think would be pretty quick to do in a couple of follow-up PRs. Also not very urgent; can be done closer to the next release.

@khaeru khaeru merged commit 1fca82f into main Mar 29, 2024
24 checks passed
@khaeru khaeru deleted the enh/2024-W13 branch March 29, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants