search

MPoL-dev / MPoL

A flexible Python platform for Regularized Maximum Likelihood imaging
https://mpol-dev.github.io/MPoL/
MIT License
33 stars 11 forks source link

V0.2.0 staging #199

Closed jeffjennings closed 10 months ago

jeffjennings commented 10 months ago

This PR does last clean-up steps to stage the codebase for v0.2.0 release.

Closes #123 Closes #148 Closes #198

jeffjennings commented 10 months ago

@iancze You might want to look through the new changelog entry and add anything that comes to mind. I think it's pretty exhaustive.

9a0dfed has the last TODOs marked; can you address those?

For the release, it looks like the approach is to merge this into main --> publish a pre-release --> make sure the pre-release.yml workflow passes --> publish a (true) release that is automatically uploaded to PyPI?

iancze commented 10 months ago

I'm getting a lot of errors on the docs build:

cd docs
make html

yields things like 

/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/fourier.py:docstring of mpol.fourier.FourierCube.from_image_properties:3: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:26: ERROR: Unexpected indentation.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:28: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.split_diagnostics_fig:4: WARNING: Inline interpreted text or phrase reference start-string without end-string.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.train_diagnostics_fig:3: ERROR: Unexpected indentation.
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:22: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:26: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:27: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:27: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:30: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:30: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:31: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:32: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:32: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:32: ERROR: Unknown interpreted text role "function".

Might want to double-check the Sphinx linking of these, and docstrings?

jeffjennings commented 10 months ago

I'm getting a lot of errors on the docs build:

cd docs
make html

yields things like

/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/fourier.py:docstring of mpol.fourier.FourierCube.from_image_properties:3: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:26: ERROR: Unexpected indentation.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:28: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.split_diagnostics_fig:4: WARNING: Inline interpreted text or phrase reference start-string without end-string.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.train_diagnostics_fig:3: ERROR: Unexpected indentation.
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:22: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:26: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:27: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:27: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:30: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:30: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:31: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:32: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:32: ERROR: Unknown interpreted text role "function".
/Users/ian/Documents/Research/Disks/RML/MPoL/docs/changelog.md:32: ERROR: Unknown interpreted text role "function".

Might want to double-check the Sphinx linking of these, and docstrings?

I see these in the workflow run for the most recent push too...I don't know why these 'ERROR's don't break the docs build and fail the test. Bah. Anyway, all fixed by 41007bd and ae09abd.

iancze commented 10 months ago

This was probably introduced by #159 and I missed it, but I get warnings on the tests for

test/onedim_test.py::test_radialV
  /Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/matplotlib/cbook/__init__.py:1369: ComplexWarning: Casting complex values to real discards the imaginary part
    return np.asarray(x, float)

test/onedim_test.py::test_radialV
  /Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/numpy/ma/core.py:2826: ComplexWarning: Casting complex values to real discards the imaginary part
    _data = np.array(data, dtype=dtype, copy=copy,

It's not a huge deal, but (for completeness of the test) it might be a good idea to separately cast to real and imag using np.real and np.imag and plot both quantities. Hopefully the imaginary parts are consistent with zero, but good to verify.

iancze commented 10 months ago

I'm getting fewer errors on the docs build, but still more than 0. And there are a few warnings, mostly related to docstrings.

/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/fourier.py:docstring of mpol.fourier.FourierCube.from_image_properties:3: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:26: ERROR: Unexpected indentation.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:28: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.split_diagnostics_fig:4: WARNING: Inline interpreted text or phrase reference start-string without end-string.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.train_diagnostics_fig:3: ERROR: Unexpected indentation.

I am wondering if these are from the mixed documentation styles confusing the parser. We've been using napolean style strings in conf.py. @jeffjennings, perhaps you might consider changing over your docstrings in crossval.py and plot.py to the napolean style, and see if this resolves?

jeffjennings commented 10 months ago

With my commits yesterday - 41007bd and ae09abd - I'm not getting any of those warnings or errors. Did you try running the docs build via make -C docs clean make -C docs html

or after pip install .?

jeffjennings commented 10 months ago

5410d2c fixes the test ComplexWarning for test_radialV by plotting Re(V) and Im(V) separately.

iancze commented 10 months ago

With my commits yesterday - 41007bd and ae09abd - I'm not getting any of those warnings or errors. Did you try running the docs build via make -C docs clean make -C docs html

or after pip install .?

Yea, I still get these errors after incorporating your commits. They are fewer than before your commits, but still non-zero. The docs build does complete, but they are output during the process. Do you not get these on your build (make html)?

/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/fourier.py:docstring of mpol.fourier.FourierCube.from_image_properties:3: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:26: ERROR: Unexpected indentation.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/crossval.py:docstring of mpol.crossval.CrossValidate:28: WARNING: Definition list ends without a blank line; unexpected unindent.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.split_diagnostics_fig:4: WARNING: Inline interpreted text or phrase reference start-string without end-string.
/Users/ian/Documents/Research/Disks/RML/venv/lib/python3.9/site-packages/mpol/plot.py:docstring of mpol.plot.train_diagnostics_fig:3: ERROR: Unexpected indentation.

In my git log I see

Date:   Thu Nov 9 09:45:24 2023 -0500

    Merge branch 'v0.2.0_staging' of https://github.com/MPoL-dev/MPoL into v0.2.0_staging

commit 5410d2ca4a368e118d94d14cd6af0b0eeaf3eae1
Author: Jeff Jennings <jjennings1519@gmail.com>
Date:   Thu Nov 9 09:45:19 2023 -0500

    test_radialV: plot real and imag separately

commit fe5270370269811f317c0b65c497123472aef853
Author: Ian Czekala <iancze@gmail.com>
Date:   Thu Nov 9 10:11:11 2023 +0000

    attempt to reconcile docstrings, but not fully there.

commit 38222cd9ed3f5469ed18412b213b2ff590e54eb2
Author: Ian Czekala <iancze@gmail.com>
Date:   Thu Nov 9 09:56:20 2023 +0000

    set sparse_matrices=False to avoid nuFFT warning with multidimensional array.

commit ae09abdfaaa9b2cbb6b97a951c6e8847ea7f9cbd
Author: Jeff Jennings <jjennings1519@gmail.com>
Date:   Wed Nov 8 12:00:14 2023 -0500

    cods build docstring complaints

commit 41007bd454f6e52dcb03f70391341aedbdf32d64
Author: Jeff Jennings <jjennings1519@gmail.com>
Date:   Wed Nov 8 11:59:54 2023 -0500

    changelog: correct {function} to {func}

and I see your changes to the docstrings locally in my source directory.

If I check mpol.__version__ after import, I see v0.2.0. I have the package installed via pip install -e ., so usually I don't need to do another pip install . after updating a branch. The errors persist after make clean, too.

I guess this isn't the biggest issue, since I can open up the built docs and they look correct. But it's still puzzling why we keep getting errors.

I do see these same errors in the logs docs build of the recent github actions, too.

jeffjennings commented 10 months ago

Yes well that's also a good point - the github actions workflow for the docs build is only triggered when a review is requested or a PR closed. I'll change that now.

I'm guessing the issue is I have a newer package version for one of the docs dependencies than mpol requires / you have installed locally. I'll check if requiring a newer version of sphinx or similar fixes the errors.

jeffjennings commented 10 months ago

Yes updating the required version for sphinx (which also forced me to update the version for sphinx_book_theme) in be9c8e7 fixed the docstring complaints. They're absent from the latest workflow report, and when I build the docs locally and browse the built html, the formatting still all looks correct to me (maybe good to double-check for you).

jeffjennings commented 10 months ago

In 4a3b9bc I've set the docs build test to trigger for: 

  pull_request:
    types: [review_requested, closed]
  pull_request_review:
      types: [submitted, dismissed]

Previously it just triggered for 

  pull_request:
    types: [review_requested, closed]

I don't think it needs to run on every push, as it's slow (5 min.), but now it will run throughout the PR review process.

iancze commented 10 months ago

Thanks for chasing that down, quite a bizarre origin for an error. Things build well on my side now. Once the GitHub Actions pass, I think this is ready to merge!

jeffjennings commented 10 months ago

Great, thanks! I've made a couple small other changes:

Once the tests pass, I'll merge!

iancze commented 10 months ago

Great! Since we have a wiki now, is there anything else from the developer docs that makes sense to put there? Or vice-versa?

jeffjennings commented 10 months ago

Good point, I think since the developer docs are user-facing, the wiki on releasing a new version doesn't need to be there, but I did add a one-line wiki 'Contributing to MPoL' that points to the developer docs.

jeffjennings commented 10 months ago

Actually I guess for anyone who comes on later as a maintainer, it's useful to have everything at least linked/referenced in the same place, so I've also just linked to the new release wiki in the developer docs.