Skip to content

Minor corrections#378

Merged
MikhailRyazanov merged 7 commits into
PyAbel:masterfrom
MikhailRyazanov:tmp
Nov 10, 2023
Merged

Minor corrections#378
MikhailRyazanov merged 7 commits into
PyAbel:masterfrom
MikhailRyazanov:tmp

Conversation

@MikhailRyazanov
Copy link
Copy Markdown
Member

@MikhailRyazanov MikhailRyazanov commented Nov 9, 2023

  1. I've noticed that Python 3.12 started to complain (correctly) about an invalid escape sequence in setup.py because a regex string wasn't prefixed with r.
  2. Supported Python versions are updated to 3.12. We can't test it automatically yet (at least it's not available for Windows in Appveyor), but it works locally, and in fact the conda packages for it have been built 2 weeks ago (without any complains so far).
  3. The README had a confusing mistake: abel.center.center_image instead of abel.tools.center.center_image (perhaps danging from some old times).
  4. I think, it would be useful to direct questions to "discussions" rather than "issues", in line with Dan's comment, so I've revised the "Support" section accordingly. Any objections?

Copy link
Copy Markdown
Collaborator

@stggh stggh left a comment

Choose a reason for hiding this comment

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

All looks good @MikhailRyazanov, thanks.

Some additional comments:

  • Item 3. abel.tools.center.center_image is correct, but abel.center_image() also works because of the content of abel/__init__.py:
 (py312)[Gibbo:.+MR-tmp/examples]$ ipython
      
 In [1]: import abel

 In [2]: abel.center_image?

I don't know whether this is a good or bad "feature".

  • Not due to any changes in this PR. I ran examples/example_all_O2.py and it failed with the error:
  File "/home/stg121/anaconda3/envs/py312/lib/python3.12/site-packages/abel/tools/polar.py", line 71, in reproject_image_into_polar
    nr = np.int(np.ceil((r.max() - r.min()) / dr))
         ^^^^^^
  File "/home/stg121/anaconda3/envs/py312/lib/python3.12/site-packages/numpy/__init__.py", line 324, in __getattr__
    raise AttributeError(__former_attrs__[attr])
AttributeError: module 'numpy' has no attribute 'int'.
`np.int` was a deprecated alias for the builtin `int`.
(py312)[Gibbo:.+-tmp/abel/tools]$ grep 'np.int' *.py
polar.py:    nr = np.int(np.ceil((r.max() - r.min()) / dr))
polar.py:        nt = np.int(np.ceil((theta.max() - theta.min()) / dt))

A convenient point to correct the above file. (Edit: Only the one file I think)

@MikhailRyazanov
Copy link
Copy Markdown
Member Author

Thanks, @stggh!
It seems that @DanHickstein has imported center_image directly to abel in a4f46f9, together with transform (which later became Transform), probably because these are the most important functions. I guess, it would then make sense to use abel.center_image() in the README and mention this alias in the center_image() docstring. (Another possibility would be to deprecate abel.center_image()...)

Regarding np.int, I've corrected them already in e5c2f86. Are you using some old versions?

@stggh
Copy link
Copy Markdown
Collaborator

stggh commented Nov 9, 2023

Regarding np.int, I've corrected them already in e5c2f86. Are you using some old versions?

Hmm, it appears so. I re-cloned your tmp and now all is well. Sorry, about that!

@stggh
Copy link
Copy Markdown
Collaborator

stggh commented Nov 9, 2023

Another possibility would be to deprecate abel.center_image()...

I agree with this.

@DanHickstein
Copy link
Copy Markdown
Member

Another possibility would be to deprecate abel.center_image()...

If I recall correctly, our thinking at the time was that center_image is such a commonly used function that, while burying it in abel.tools.center.center_image() makes sense from the viewpoint of code-organization, it was too annoying for users. Currently we can access the function via both abel.center_image and abel.tools.center.center_image, correct? In my view this is the desirable behavior. I think adding a statement to the docstring to indicate the shortcut is a good idea.

But, my feelings on the issue are not too strong, so I could be convinced otherwise.

And directing people to the Discussions feature seems like a reasonable idea. If we don't like it, it's easy enough to switch back to Issues.

Thanks again for keeping PyAbel up-to-date @MikhailRyazanov!

@MikhailRyazanov
Copy link
Copy Markdown
Member Author

A difficult decision... :–)
In principle, I agree with Dan that allowing abel.center_image() is more user-friendly than requiring abel.tools.center.center_image() (and centering an image before transforming it might be useful for sanity checks, although for this purpose, the centered input image is also available as Transform.IM). On the other hand, Steve, as a real user, doesn't think that the shorthand is necessary; and I don't know whether anybody at all uses it, since it wasn't documented anywhere.
We can flip a coin, if nobody has strong arguments either way. :–D

@stggh
Copy link
Copy Markdown
Collaborator

stggh commented Nov 9, 2023

Steve, as a real user

Not so much these days. No new data from my lab, as it has been closed down, and the spectrometer dismantled :-(

abel.tools.center.center_image()

I prefer that the explicit call provides the directory location of the function (and API)

Not a big deal. I am happy with any call.

@MikhailRyazanov
Copy link
Copy Markdown
Member Author

OK, I'll merge this as is (using the full path in the README), and let's discuss in #379 what should be done with abel.center_image().

@MikhailRyazanov MikhailRyazanov merged commit b10237d into PyAbel:master Nov 10, 2023
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