Fix issues reported by pyflakes and add pyflakes step to ci, fixes #3756

[maxnoe]

Description

This fixes all issues reported by pyflakes in the gammapy package. Most of them were related to star imports and missing __all__ statements but some also were genuine issues, unused imports, f-strings without placeholders, unused variables and the like.

[codecov]

Codecov Report

Merging #3760 (67ce3d4) into master (43af844) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3760   +/-   ##
=======================================
  Coverage   93.77%   93.78%           
=======================================
  Files         162      162           
  Lines       19576    19595   +19     
=======================================
+ Hits        18358    18377   +19     
  Misses       1218     1218           

Impacted Files	Coverage Δ
gammapy/astro/darkmatter/profiles.py	100.00% <ø> (ø)
gammapy/astro/population/simulate.py	95.83% <ø> (ø)
gammapy/astro/population/spatial.py	82.11% <ø> (ø)
gammapy/astro/population/velocity.py	100.00% <ø> (ø)
gammapy/catalog/fermi.py	95.29% <ø> (ø)
gammapy/catalog/hess.py	97.69% <ø> (ø)
gammapy/makers/utils.py	98.79% <ø> (ø)
gammapy/modeling/iminuit.py	92.52% <ø> (ø)
gammapy/modeling/models/cube.py	88.41% <ø> (ø)
gammapy/modeling/scipy.py	97.05% <ø> (ø)
... and 50 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43af844...67ce3d4. Read the comment docs.

[adonath]

Thanks @maxnoe! I think the mostly looks fine, except the places where the * import is replaced with the explicit imports. While it is in general true that one should avoid the * import, it could be considered a false positive in this case. The point is that one does not need both:

• Either the * import and __all__ to define what is actually visible in the sub-package namespace
• Or the explicit imports, but then there is no need for __all__ anymore, because the explicit imports define what is actually visible in the namespace

I have a slight preference for the old scheme with a combination of * and __all__, but the explicit import would work equally well, but then one could get rid of the __all__?

[maxnoe]

Either the * import and all to define what is actually visible in the sub-package namespace
Or the explicit imports, but then there is no need for all anymore, because the explicit imports define what is actually visible in the namespace

That is only true for python import semantics. It is not true for sphinx automodapi, I think. That one requires __all__ to correctly specify the classes as it uses that to decide what gets documented, if I am not mistaken. At least we had several issues in the ctapipe docs with star imports and things missing from __all__.

I would have to check if the autogenerated docs changed...

[adonath]

@maxnoe It seems there is a autosummary_imported_members option for conf.py, which addresses this? See https://github.com/sphinx-doc/sphinx/pull/6212.

[maxnoe]

It seems there is no way of letting pyflakes (or pylint) resolve the star imports. Which is a bit sad...

[Bultako]

Thanks @maxnoe

I think in general this PR is needed, and in particular adding pyflakes to the CI, not only for linting purposes but also to build a better API documentation. At this moment some classes are missing from the API docs, and this PR could help in fixing this issue.

@adonath
I have tested just adding the autosummary_imported_members=True flag to docs/conf.py with no better results, still the same classes missing from the API docs.

I would go with this PR, but it still needs some modifs.

• remove duplicated classes in __all__ which yields duplication of classes also in the API docs lists.
• sort the classes in __all__ alphabetically so they could be more easy to find (i.e. see modelling API)

[Bultako]

other differences found in the API docs produced with this PR:

• Parameter and Parameters listed in modeling but with no links.
• SourceCatalog3HWC and SourceCatalog2HWC missing from catalog package.
• TRUNCATION_VALUE variable missing in stats

[maxnoe]

Parameter and Parameters listed in modeling but with no links.

This is probably because they are defined in a higher level module and sphinx is not able to figure that out. That's quite unusual. They are correctly linked above in gammapy.modeling but not in gammapy.modeling.models.

I think the best solution would be to just remove these two classes from the lower level namespace gammapy.modeling.models and fix the one notebook where they are imported from there.

[maxnoe]

TRUNCATION_VALUE variable missing in stats

Is this really something that should be documented? It's a module level constant in a cython extension.

SourceCatalog3HWC and SourceCatalog2HWC missing from catalog packag

fixed

[adonath]

Thanks @Bultako for checking! So we just probably keep was @maxnoe is proposing here. It's non-ideal because it increases the effort of bookkeeping (e.g. if new classes and methods are added, they have to be added in 2 places in addition to the actual implementation), but complete documentation is more important.

[bkhelifi]

@adonath can we precise then this in the Developer documentation? Because it is not so obvious...

[Bultako]

Thanks @maxnoe
There are still some things to fix:

• rebase since it seems there are conflicts with master
• still some work to do in alphabetical order in __all__ vars (i.e. modeling.models, estimators..)
• what to do with not linking to Parameter and Parameters?

From my side if you just rebase we could merge this PR, I could open a new one to fix ordering and the links to Parameters classes.

[maxnoe]

rebase since it seems there are conflicts with master

Done

still some work to do in alphabetical order in all vars (i.e. modeling.models, estimators..)

modeling.models was already sorted. estimators sorted now.

what to do with not linking to Parameter and Parameters?

I'd suggest to remove both classes from gammapy.modeling.models and fix the one notebook where they are imported from there and not from gammapy.modeling

[Bultako]

Thanks @maxnoe
I've written some comments in between the lines of code to finish with the alphabetical sorting. As for your proposed solution to link to the Parameter and Parameters docstrings, you can do it in this PR or in another different one. It's up to you.

[maxnoe]

@Bultako Went over it again, hope it's ok now

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[Bultako]

@maxnoe
It seems most of the comments I made in between the lines have not been solved :(
Just copy/paste the content of the __all__ var that I propose in those not solved comments.

[maxnoe]

@Bultako I went over all __all__s and sorted again with case insensitive sorting....

If that doesn't match what you proposed, you did some manual ordering?

[Bultako]

@maxnoe
If we compare the content of the unresolved comments and actual code in the files changed tab we can see that the ordering has not been done properly.
Yes, I did it manually.

[maxnoe]

Yes, I did it manually.

Manually according to which rules? You asked for alphabetic sorting. Yet you diverged from it apparently. So we need to define a sorting criteration which developers can follow, if we don't want that to be alphabetic.

[maxnoe]

Ok, sorry, it seems I missed some files due to the limit on how many tabs neovim can open at the same time....

Now everything containing an __all__ should be sorted correctly.

For future reference, this does this automatically:

rg __all__ -l | sort | uniq | xargs -i nvim {} +'g/__all__ = \[$/+1,/\]/-1 sort ui' -c wq 

[Bultako]

Thanks @maxnoe
It seems it worked now :)
If you can replace the single quotes by double quotes in the __all__ lists, we merge it 👍🏻

[maxnoe]

Done

[Bultako]

Thanks @maxnoe !
merging now

[adonath]

Thanks @maxnoe and @Bultako!