Disabled FITS & UnitsWarnings by default for ESASky module #2550
Conversation
…be enabled via the new verbose parameter
Codecov Report
@@ Coverage Diff @@
## main #2550 +/- ##
==========================================
- Coverage 63.99% 63.97% -0.02%
==========================================
Files 132 132
Lines 16969 16984 +15
==========================================
+ Hits 10859 10866 +7
- Misses 6110 6118 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
astroquery/esasky/core.py
Outdated
| verbose=verbose, | ||
| if not verbose: | ||
| commons.suppress_vo_warnings() | ||
| warnings.filterwarnings("ignore", category=astropy.units.core.UnitsWarning) |
There was a problem hiding this comment.
This is modifying the warnings filter globally, it would be better to use the warnings.catch_warnings() context manager.
There was a problem hiding this comment.
Thanks for teaching me something new today! I've now added a context manger. Do you have any opinions regarding the suppression of specific warnings like done above or if it is more in line with the astroquery guidelines to use warnings.simplefilter("ignore") instead? @bsipocz
There was a problem hiding this comment.
Recently I did this very specific ignore for windows, and don't have a better suggestion for this case either. Make the ignore module or small code part specific, and be as specific as possible to not overcatch and ignore warnings:
https://github.com/astropy/astroquery/blob/main/astroquery/sdss/core.py#L940
As for units related issues, other modules are casting them over for a fix (e.g. nasa_exoplanet_archive), but I think that's less applicable for the esasky case as here you may run into much more, and less predictable unit issues (for them it's about one set of units the remote services uses that they need to cast into valid astropy units, while here it's some non standard compliancy of the votables in the response).
There was a problem hiding this comment.
Alright! Let's keep it as is then. It is unfeasable for ESASky to repair FITS file data, since ESASky is exposing data from so many missions.
astroquery/esasky/core.py
Outdated
| if verbose: | ||
| return fits.open(path) | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") |
There was a problem hiding this comment.
hmm, we may overcatch with this one
There was a problem hiding this comment.
Maybe... Since ESASky exposes FITS files from so many missions, I feel that we still need a quite broad filter. I did some quick searches in astropy.io.fits and it seems that ignoring VerifyWarning might be better suited for this use case. That would let through AstropyDeprecationWarning, AstropyUserWarning, and potentially others.
There was a problem hiding this comment.
Yes, start with something small, and we can always add other specific ones. Otherwise, we indeed would miss the important and relevant ones, like deprecations, etc.
astroquery/esasky/core.py
Outdated
| commons.suppress_vo_warnings() | ||
| warnings.filterwarnings("ignore", category=astropy.units.core.UnitsWarning) | ||
| job = self._tap.launch_job(query=query, output_file=output_file, output_format=output_format, | ||
| verbose=verbose, dump_to_file=output_file is not None) |
There was a problem hiding this comment.
Extreme nitpick, but you're already inside the conditional, I think it's fair to use verbose=False here? and verbose=True in the else.
As suggested by @bsipocz here, some warnings are not useful to the users. Thus, the suggested warning as well as some extra warnings are now disabled by default, but can be enabled via the verbose parameter.
Also, some minor pep8 formatting.