SNEWS2 / snewpy

A Python package for working with supernova neutrinos
https://snewpy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
24 stars 17 forks source link

`Mori_2023` raises errors #316

Closed JostMigenda closed 2 months ago

JostMigenda commented 2 months ago

Two separate issues I noticed while trying to use the recently added Mori_2023 model from sntools:


The demo notebook Mori_2023.ipynb fails to run:

> from snewpy.models.ccsn import Mori_2023
> model_std = Mori_2023(axion_mass=0, axion_coupling=0)

File ~/opt/miniconda3/envs/snewpy/lib/python3.12/site-packages/snewpy/models/ccsn.py:458, in Mori_2023.__init__(self, axion_mass, axion_coupling)
    443 # PNS mass table, from Mori+ 2023.
    444 mpns = { (0, 0):    1.78,
    445          (100, 2):  1.77,
    446          (100, 4):  1.76,
   (...)
    456          (200, 10): 1.73,
    457          (200, 20): 1.62 }
--> 458 am = int(axion_mass.to_value('MeV'))
    459 ac = int(axion_coupling.to_value('1e-10/GeV'))
    460 pns_mass = mpns[(am,ac)]

AttributeError: 'int' object has no attribute 'to_value'

It’d be nice to let users enter 0 instead of requiring 0 * u.MeV, so we should add code to handle this special case.


Loading a model directly from file via

from snewpy.models.ccsn_loader import Mori_2023
model = Mori_2023("/path/to/input_file")

fails with a KeyError, since the loader class extracts entries from the metadata dictionary. However, metadata is an optional argument (like for all loader classes), so if it isn’t present, we should handle that gracefully.

(Of course, the loader classes are not a public API; but they’re useful for internal testing/usage.)


I’m happy to fix both; just one question @sybenzvi: Is there a particular reason to add self.axion_mass etc. as instance attributes, rather than accessing them via self.metadata['axion_mass']? This looks like unnecessary duplication to me; and we don’t generally do this for metadata entries in other models.

mcolomermolla commented 2 months ago

Actually, this comment applies to all models. Right now, they all require not only the value but also the units for the model initialization. If we want to be self-consistent, this model should also keep the u.MeV after the value, or we should change that for all models accordingly.

JostMigenda commented 2 months ago

@mcolomermolla I wouldn’t mind treating 0 as a special case here, since a mass of 0 is perfectly clear even without units; whereas e.g. a mass of 100 is meaningless without units. So I don’t think this would apply to most other models? But you’re right; it’d be more consistent to require units everywhere and just update the notebook.

sybenzvi commented 2 months ago

I created PR #317 to unbreak the notebook. About loading the model directly from a file, you're right that the model is set up to take axion_mass and axion_coupling as required parameters and is duplicating the loader. Do you want to have a go at that @JostMigenda ?