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

Allowing for negative extinction when plotting too #123

Closed
gabrielastro opened this issue Oct 26, 2024 · 6 comments
Closed

Allowing for negative extinction when plotting too #123

gabrielastro opened this issue Oct 26, 2024 · 6 comments
Labels
type: feature New feature or request

Comments

@gabrielastro
Copy link

Hello Tomas,

Hurt et al. (2024) allowed for negative amounts of extinction AV, which gave better fits in several cases. The justification is:

Including reddening in our model may lead to better fits if
BT-Settl accounts for too little or too much extinction from
dust in an object’s atmosphere. Positive values of A V would
indicate that more extinction from dust is needed to explain
the object’s spectrum, whereas negative values would suggest
that less extinction is needed.

That does sound very physical. Currently, it works in species when doing the spectral fitting but not when plotting the spectrum (!), as it leads to

Opening ModelBox...
…
wavelength = []
flux = []

which of course if isinstance(wavelength[0], (np.float32, np.float64)): does not like, etc..

Would it be possible to allow for this? Thanks!

Gabriel

@tomasstolker tomasstolker added the type: feature New feature or request label Oct 30, 2024
@tomasstolker
Copy link
Owner

Thanks for the suggestion! Using negative extinction values should now be possible with ReadModel and FitModel. Please give it a try and let me know if you run into any issues.

@gabrielastro
Copy link
Author

Thanks! First of all, with the recent updates to species, I get: ModuleNotFoundError: No module named 'spectres.spectral_resampling_numba'. I figured I should update spectres. Then, I needed to install PyAstronomy, which lead to problems with setuptools (and packaging) similar to what is described in a few fora, for example here. Using pip3.11 install --upgrade setuptools==70.0.0 (if it is too recent, e.g. v. 75, there are problems!), and then re-doing the similar install for the other packages, finishing with pip3.11 install -e . in my checked-out species directory, eventually solved it. Just to say: these are external things for which you are obviously not responsible but I am wondering whether this could be automated.

Now trying again my script, I get an error pointing to species/read/read_model.py:961:

--> 961     model_box.flux = self.apply_ext_ism(
    962         model_box.wavelength,
    963         model_box.flux,
    964         ism_ext_av,
    965         ism_reddening,
    966     )

namely:

TypeError: type of the return value must be a tuple; got numpy.ndarray instead

I have both ism_ext and ism_red.

Actually, that was accidentally using python 3.8, but now using the correct version (using python3.11 -m IPython --matplotlib to use 3.11 instead of ipython, which defaults to the system default of 3.8—and better not change that one in Ubuntu to not break logging in, opening a terminal, etc.!), I still get very similarly:

TypeCheckError: the return value (numpy.ndarray) is not a tuple

coming from return flux * 10.0 ** (-0.4 * ext_mag) in species/read/read_model.py:569. Is it "my fault" 🙂?

@tomasstolker
Copy link
Owner

There was indeed an issue with the return, which should be fixed now!

@gabrielastro
Copy link
Author

I tested it and now it is perfect. Thank you very much, also for your swift action!

@gabrielastro
Copy link
Author

gabrielastro commented Nov 5, 2024

I will just an add that it might be good to rename ism_ext and ism_red to ext_av and ext_redv (or ext_rv but that could confusingly sound like "external radial velocity", whatever that is) to have it more general, especially if using some atmospheric opacity as in Bonnefoy et al. (2016) or Hirakana et al. (2016)… [edit:] as might be possible in the future 😉 and sort of already now thanks to lognorm_radius or powerlaw_max et al..

@tomasstolker
Copy link
Owner

Yes I agree, these name are not ideal, but will leave them for now. At some point, I want to implement a more generic approach for the extinction, which requires refactoring.

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

No branches or pull requests

2 participants