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

Change Fornax_2022 input parameters #293

Closed Sheshuk closed 5 months ago

Sheshuk commented 7 months ago

I missed the discussion in #283, and looking at the new model class now. It has two input parameters progenitor and progenitor_mass which, if provided, should match each other (checked by validator): https://github.com/SNEWS2/snewpy/blob/7a2176ffa54f95ca987bcf7ae1c16bfd578b6aeb/python/snewpy/models/ccsn.py#L773-L779

I it makes rather complicated for a user: providing two highly correlated parameters mean there is a lot of space for errors and/or requires copy-paste in the constructor invocation.

I understand that this is done because some of the simulations form the black hole.

From the user interface point of view I see two options for improvement:

  1. Make parameters progenitor_mass:u.Quantity, black_hole:bool, and change the description for progenitor_mass values, listing separately the valid values for black_hole==False and then for black_hole==True.
  2. Split this model class in two separate classes, for example: Fornax_2022_sn and Fornax_2022_bh each having a single parameter progenitor_mass. We can also put the black_hole=True to the metadata in these classes, to be more informative.

What do you think?

Sheshuk commented 7 months ago

Another (the easiest) option would be to

  1. Keep just the progenitor parameter.

But it's not physical, but rather an inner representatation of the models, and in this sense is not that different from just providing the filename directly. So initializing from progenitor_mass would be better. So I personally prefer the option 2

JostMigenda commented 7 months ago

Would a separate black_hole parameter actually add any information? AFAICT, we never have a case where two progenitors have the same mass, but one forms a BH and the other doesn’t; so it would be completely sufficient to have a single progenitor_mass parameter. Whether or not a given progenitor forms a BH is useful information that we can include in the metadata, without requiring the user to provide it.

Note that this is consistent with what we already do for various other models. E.g. some (but not all) Warren_2020 or Nakazato_2013 progenitors form a BH; but they are uniquely identified by other progenitor properties and, so we don’t require users to provide that duplicate information.

mcolomermolla commented 7 months ago

I agree with Jost in principle that this info does not bring anything as for one progenitor mass, it whether forms or not a BH, but we won't have two progenitors with same mass, one forming a bh and the ther not. But the problem here I think is that without the "bh" info, in the case of the Fornax_2022 models, the model inititaliser wouldn't know which file to download and read, so it is a needed parameter somehow.