Skip to content
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.

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

HLA-1419: Removal of warnings for photometry_utils.py log of zero of negative number #1956

Conversation

s-goldman
Copy link
Collaborator

@s-goldman s-goldman commented Feb 24, 2025

Resolves HLA-1419

This PR addresses warnings that are printed too often. The warning shows up in the calculation of catalog magnitudes during the creation of the catalogs (SVMs). The calculation of the magnitudes uses the log of the fluxes. Sometimes the fluxes are negative (not real) or set to zero. The warning is from calculating the log10 of a negative number or zero.

This solution just ignores these warnings. A situation may arise where bad values may creep into the catalog and be missed by dismissing these warnings. This error would be immediately identified as the flux values are also in the catalog. As a result, this should not cause concern.

Checklist for maintainers

  • added entry in CHANGELOG.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant label(s)

@s-goldman s-goldman requested review from mdlpstsci and a team as code owners February 24, 2025 19:55
abmag = stmag - 5.0 * np.log10(photplam) + 18.6921
# ignores warnings from log10 of zeros and negative values
with np.errstate(divide='ignore', invalid='ignore'):
abmag = stmag - 5.0 * np.log10(photplam) + 18.6921
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lines 241 and 246 introduce benign changes which eliminate lots of messages in the logs without changing the current computations. In this sense, the changes are fine. However, the Jira ticket was meant to investigate how these computations can be better handled starting from the routine which calls compute_flux_to_abmag. Should the block of code starting on Line 231 be expanded to encompass handling the flux array? If issues are handled in the initial block of code we never drop down into the log10 computations.

In the test dataset was photplam zero or negative? It is a pivot wavelength, so I am curious.

mag_err = 1.0857 * flux_error / flux
# ignores warnings from log10 of zeros and negative values
with np.errstate(divide='ignore', invalid='ignore'):
mag_err = 1.0857 * flux_error / flux
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like the flux here is the troublesome variable as there is no log computation. What is the value of mag_err here after the divide by zero?

Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

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

This ticket was meant to be an investigation to understand (beyond zero or negative values) how to handle better these issues. This requires some tracing back in the code to the calling functions. If this is expanding into too much of a research ticket, then it should be put back on the backlog. I do not want to mask the issues just yet, as I think the issues should actually be handled better than they are now. What I am suggesting is potentially a more invasive change.

@s-goldman s-goldman closed this Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants