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

[BUG] "Counts conversion factor" and "Flux scaling" are 0 by default instead of 1 #2550

Open
eteq opened this issue Nov 1, 2023 · 8 comments
Labels
feature Feature request imviz

Comments

@eteq
Copy link
Contributor

eteq commented Nov 1, 2023

Jdaviz component

Imviz

Description

"Counts conversion factor" and "Flux scaling" options of the aperture photometry plugin are 0 when you open up apphot. Both, though, seem like they should be 1 not 0 to be the case of "ignore those correction factors".

How to Reproduce

  1. Install jdaviz
  2. Open up aperture photometry plugin

Expected behavior

I would have expected these two scale factors to be 1 as the no-op case, since 0 should lead to divide-by-zero errrors. It does not, so I guess 0 must be a placeholder for "ignore", but that's confusing.

Browser

No response

Jupyter

IPython : 8.16.0
ipykernel : 6.25.2
ipywidgets : 8.1.1
jupyter_client : 7.0.6
jupyter_core : 5.3.2
jupyter_server : 1.24.0
jupyterlab : 4.0.6
nbclient : 0.8.0
nbconvert : 7.8.0
nbformat : 5.9.2
notebook : 7.0.4
qtconsole : 5.4.3
traitlets : 5.10.1

Software versions

Python 3.11.5 (main, Sep 2 2023, 14:16:33) [GCC 13.2.1 20230801]

import numpy; print("Numpy", numpy.version)
Numpy 1.26.0
import astropy; print("astropy", astropy.version)
astropy 5.3.1
import matplotlib; print("matplotlib", matplotlib.version)
matplotlib 3.8.0
import scipy; print("scipy", scipy.version)
scipy 1.11.3
import skimage; print("scikit-image", skimage.version)
scikit-image 0.21.0
import asdf; print("asdf", asdf.version)
asdf 2.15.0
import stdatamodels; print("stdatamodels", stdatamodels.version)
stdatamodels 1.6.0
import gwcs; print("gwcs", gwcs.version)
gwcs 0.18.3
import regions; print("regions", regions.version)
regions 0.7
import specutils; print("specutils", specutils.version)
specutils 1.11.0
import specreduce; print("specreduce", specreduce.version)
specreduce 1.3.0
import photutils; print("photutils", photutils.version)
photutils 1.8.0
import astroquery; print("astroquery", astroquery.version)
astroquery 0.4.6
import yaml; print("pyyaml", yaml.version)
pyyaml 6.0.1
import asteval; print("asteval", asteval.version)
asteval 0.9.30
import idna; print("idna", idna.version)
idna 3.4
import traitlets; print("traitlets", traitlets.version)
traitlets 5.10.1
import bqplot; print("bqplot", bqplot.version)
bqplot 0.12.39
import bqplot_image_gl; print("bqplot-image-gl", bqplot_image_gl.version)
bqplot-image-gl 1.4.11
import glue; print("glue-core", glue.version)
glue-core 1.12.0
import glue_jupyter; print("glue-jupyter", glue_jupyter.version)
glue-jupyter 0.17.0
import glue_astronomy; print("glue-astronomy", glue_astronomy.version)
glue-astronomy 0.10.0
import echo; print("echo", echo.version)
echo 0.8.0
import ipyvue; print("ipyvue", ipyvue.version)
ipyvue 1.9.2
import ipyvuetify; print("ipyvuetify", ipyvuetify.version)
ipyvuetify 1.8.10
import ipysplitpanes; print("ipysplitpanes", ipysplitpanes.version)
ipysplitpanes 0.2.0
import ipygoldenlayout; print("ipygoldenlayout", ipygoldenlayout.version)
ipygoldenlayout 0.4.0
import ipypopout; print("ipypopout", ipypopout.version)
ipypopout 1.0.0
import jinja2; print("Jinja2", jinja2.version)
Jinja2 3.1.2
import voila; print("voila", voila.version)
voila 0.4.1
import vispy; print("vispy", vispy.version)
vispy 0.13.0
import sidecar; print("sidecar", sidecar.version)
sidecar 0.5.2
import jdaviz; print("Jdaviz", jdaviz.version)
Jdaviz 3.5.0

@pllim
Copy link
Contributor

pllim commented Nov 1, 2023

There is actually a subtle difference between 1 and 0, and 0 is the default in the code on purpose. This is not a bug. If you look at Point 8 in https://jdaviz.readthedocs.io/en/latest/imviz/plugins.html#aperture-photometry , it says, "Setting this to 1 is equivalent to not applying any scaling. This field is only used if data has a valid unit. If this field is not applicable for you, leave it at 0."

tl;dr

  • 0 (default) -- Do not even convert to magnitude.
  • 1 -- Convert to magnitude but without any zeropoint scaling (usually this is raw instrumental magnitude).

This is because I want people to stop and think about what magnitude system they really want. I do not want people to blindly take an instrumental mag and think it is STMAG and so on.

@pllim
Copy link
Contributor

pllim commented Nov 1, 2023

Same deal with counts conversion factor. Counts can be ambiguous, so 0 means I do not do it at all. And if you request for it to be done, you better know what you are doing.

@pllim pllim closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2023
@pllim pllim added question Further information is requested imviz labels Nov 1, 2023
@kecnry
Copy link
Member

kecnry commented Nov 2, 2023

Would it be more clear if we had a switch to enable/disable and if enabled, then you can access changing the value? Or we could just be more descriptive in the hint text in the UI.

@camipacifici
Copy link
Contributor

I understand Erik's concern and I kind of agree (although it did not hit me before).
How about N/A (or some equivalent) instead of 0 when it is not in use?

@pllim
Copy link
Contributor

pllim commented Nov 2, 2023

Then we have to define it as "Any" instead of "Float", which is more processing for that one thing.

@pllim
Copy link
Contributor

pllim commented Nov 2, 2023

If the concern here is just about how it is presented in the GUI, then I prefer Kyle's way with the front-end toggles.

@kecnry
Copy link
Member

kecnry commented Nov 2, 2023

Let's reopen this then, I think there is definitely some improvements we could make, right?

@pllim pllim reopened this Nov 2, 2023
@camipacifici camipacifici reopened this Nov 2, 2023
@pllim pllim added feature Feature request and removed question Further information is requested labels Nov 2, 2023
@pllim
Copy link
Contributor

pllim commented Nov 2, 2023

Once you decide how you want this done, please update the title and open a JIRA ticket. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request imviz
Projects
None yet
Development

No branches or pull requests

4 participants