Skip to content

utils: minor code clean up #1800

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.

Sign up for GitHub

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

Jump to bottom
Merged
wmclaugh merged 4 commits into from
Jun 29, 2023
Merged

utils: minor code clean up #1800

wmclaugh merged 4 commits into from
Jun 29, 2023

Conversation

@DougBurke
Copy link
Contributor

@DougBurke DougBurke commented Jun 14, 2023 •
edited
Loading

Summary

Internal clean up of the sherpa.ui.utils module.

Details

Created when working on something else.

  • Clean up the use of _clean_model (make more use of it) and add a little documentation.
  • Avoid using a fully-qualified name when we have the symbol imported anyway.
  • use idval rather than id for an internal variable name (although the API uses id so that is left as is)
  • clean up of logging to use lazy formatting (%s) rather than f-strings
  • change a logging call to use warning(..) rather than info("WARNING: ...")

Technically this last one is a user change - it they had used SherpaVerbosity("WARNING") then the message is now seen when it wasn't before, but I don't think we need to explain this. Or we could also just not take the last commit, where this change is made.

Example

CIAO 4.15

sherpa In [1]: load_arrays(1, [1, 2, 3], [1, 2, 3])

sherpa In [2]: set_source(xsphabs.gal * xsapec.src)

sherpa In [3]: guess(gal)
WARNING: No guess found for xsphabs.gal

sherpa In [4]: guess(src)

sherpa In [5]: guess(gal + src)
WARNING: No guess found for (xsphabs.gal + xsapec.src)

sherpa In [6]: from sherpa.utils.logging import SherpaVerbosity

sherpa In [7]: with SherpaVerbosity("ERROR"):
          ...:     guess(gal)
          ...: 

sherpa In [8]: with SherpaVerbosity("WARNING"):
          ...:     guess(gal)
          ...:

This PR

In [1]: from sherpa.astro.ui import *
WARNING: imaging routines will not be available, 
failed to import sherpa.image.ds9_backend due to 
'RuntimeErr: DS9Win unusable: Could not find ds9 on your PATH'

In [2]: load_arrays(1, [1, 2, 3], [1, 2, 3])

In [3]: set_source(xsphabs.gal * xsapec.src)

In [4]: guess(gal)
WARNING: No guess found for xsphabs.gal

In [5]: guess(src)

In [6]: guess(gal + src)
WARNING: No guess found for (xsphabs.gal + xsapec.src)

In [7]: from sherpa.utils.logging import SherpaVerbosity

In [8]: with SherpaVerbosity("ERROR"):
   ...:     guess(gal)
   ...: 

In [9]: with SherpaVerbosity("WARNING"):
   ...:     guess(gal)
   ...: 
WARNING: No guess found for xsphabs.gal

@DougBurke DougBurke mentioned this pull request Jun 14, 2023
Merged
@codecov
Copy link

codecov bot commented Jun 14, 2023 •
edited
Loading

Codecov Report

Merging #1800 (a1d19a6) into main (5017a8c) will increase coverage by 1.47%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
+ Coverage   79.65%   81.13%   +1.47%     
==========================================
  Files          72       73       +1     
  Lines       25981    26338     +357     
  Branches     3953     3952       -1     
==========================================
+ Hits        20695    21369     +674     
- Misses       4449     4806     +357     
+ Partials      837      163     -674
Impacted Files Coverage Δ
sherpa/ui/utils.py 96.09% <100.00%> (+0.88%) ⬆️

... and 47 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

DougBurke and others added 3 commits June 14, 2023 10:29
@DougBurke
Tests: ensure warning message is checked
633a243 
Check that the warning message when a "full model" is over-written
is checked.
@DougBurke
utils: minor code clean up
e1c90b9 
Avoid using a fully-qualified name when we have the symbol
imported anyway.

Clean up the use of _clean_model (make more use of it) and add a
little documentation.
@DougBurke
utils: change from info to warning for guess logging
1619d7b 
Given that the message started out with WARNING: we may as well
make this a warning rather than an info log message.
@DougBurke DougBurke force-pushed the utils-minor-cleanup branch from 3dc9739 to 1619d7b Compare June 14, 2023 15:24
@DougBurke DougBurke added the note: easy review Indicates PR requires simple review label Jun 14, 2023
@DougBurke DougBurke requested a review from hamogu June 14, 2023 17:26
@hamogu
Copy link
Contributor

hamogu commented Jun 20, 2023

I agree with the last commit. For my own education, why do you prefer "%s" over format for logging? I mean, I con't care, but I'd like to learn what makes you prefer one over the other here.

hamogu
hamogu reviewed Jun 20, 2023
View reviewed changes
hamogu
hamogu reviewed Jun 20, 2023
View reviewed changes
hamogu
hamogu reviewed Jun 20, 2023
View reviewed changes
@hamogu
Copy link
Contributor

hamogu commented Jun 20, 2023

Apart from the three small comments, I think this is good to go; I'll approve once those are resolved. (Please ping if I miss it.)

@DougBurke
Copy link
Contributor Author

DougBurke commented Jun 20, 2023

For #1800 (comment) 👍

For our code I don't think there's really much difference between the two lines

info("foo bar %s blah blah", self.get_foo_bar(bob))
info(f"foo bar {self.get_foo_bar(bob)} blah blah")

but pylint likes to warn about it with it's logging not lazy warning. You can tell it not to warn about the use of f-strings for this (but then I don't understand why they allow this form, which is definitely not lazy, but that's for another day) so we could go with the latter.

One "good" thing with this change is that it made me find and fix an issue with our logging override code, but that's been done now, so it's mainly a "style" thing and not something I think we need to codify (i.e. other people don't need to write logging calls like I do).

@DougBurke @hamogu
Apply suggestions from code review
a1d19a6 
Co-authored-by: Hans Moritz Günther <moritz.guenther@gmx.de>
@DougBurke
Copy link
Contributor Author

DougBurke commented Jun 21, 2023 •
edited
Loading

Note that the tests fail because I haven't rebased this to include #1807

edited to add or not, it appears.

@wmclaugh wmclaugh merged commit 767f516 into sherpa:main Jun 29, 2023
@DougBurke DougBurke deleted the utils-minor-cleanup branch July 17, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hamogu hamogu hamogu approved these changes

Assignees

No one assigned

Labels

area:code note: easy review Indicates PR requires simple review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DougBurke @hamogu @wmclaugh