Skip to content

PISN & type Ia models - On hold - #415

Draft
jpkneller wants to merge 55 commits into
mainfrom
jpkneller-pisn-models
Draft

PISN & type Ia models - On hold#415
jpkneller wants to merge 55 commits into
mainfrom
jpkneller-pisn-models

Conversation

@jpkneller

Copy link
Copy Markdown
Contributor

New pisn model classes and Jupyter workbook

New pisn model classes and Jupyter workbook
pisn classes and loaders
@jpkneller jpkneller changed the title Add files via upload PISN models Nov 6, 2025
@jpkneller
jpkneller marked this pull request as ready for review June 1, 2026 14:53
This was referenced Jun 10, 2026
@JostMigenda JostMigenda changed the title PISN models PISN & type Ia models Jun 10, 2026
@JostMigenda

Copy link
Copy Markdown
Member

I’ll try to review this in details tomorrow; here are some initial, high-level thoughts:

  • For both models, tests are missing.
  • The loader classes for both models look nearly identical; which makes sense given they’re the exact same format. Are there relevant differences or would it make sense to de-duplicate that?
  • The TypeIa class doesn’t match our Lastname_YYYY convention for model classes. Can we rename that to Wright_2016? (Which matches the year both preprints appeared on the arXiv; even if the PRD paper on the GCD model is from 2017.)
  • I notice the files for PISN and type Ia are in a combined repo, but the classes are in separate files. I don’t have a strong preference either way; happy to leave it as is, but wanted to flag it in case people see advantages to one or the other. (If joint, maybe snewpy-models-exotics/snewpy.models.exotics would be a better name, just in case there’s ever any other exotic models.)

Comment thread python/snewpy/models/pisn_loaders.py Outdated
Comment on lines +111 to +117
#convert input arguments to 1D arrays
t = u.Quantity(t, ndmin=1)
E = u.Quantity(E, ndmin=1)

initial_spectra = {}
for flavor in ThreeFlavor:
initial_spectra[flavor] = self.interpolation[flavor]((t, E)) / (u.MeV * u.s)

@JostMigenda JostMigenda Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If both t and E are multi-element arrays (e.g. if we do model.get_flux(times, energies, d) as demonstrated in FlavorTransformation.ipynb), this will fail.

The fix could be something like:

        t = u.Quantity(t, ndmin=1).to(u.s).value
        E = u.Quantity(E, ndmin=1).to(u.MeV).value
        tE_grid = np.stack(np.meshgrid(t, E, indexing='ij'), axis=-1)

        initial_spectra = {}
        for flavor in ThreeFlavor:
            initial_spectra[flavor] = self.interpolation[flavor](tE_grid) / (u.MeV * u.s)

(Note that we also need to remove the units in between, otherwise np.stack will complain that we’re mixing units.)

I’ve played around with a toy example to figure out the correct order of indices (self.interpolation[flavor](tE_grid)[i, j] == self.interpolation[flavor]((t[i], E[j]))), but since this is so easy to mix up, I would appreciate if someone could double-check that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The same applies to the TypeIa class.

Moving SNOWGLOBES model class to base.py. Will become parent to TypeIa and Wright_2017 model classes.
Moved SNOwGLoBES model class here. It will become the parent for the TypeIa and Wright_2017 model classes.
@jpkneller

Copy link
Copy Markdown
Contributor Author
* For both models, tests are missing.

I'll fix that.

* The loader classes for both models look nearly identical; which makes sense given they’re the exact same format. Are there relevant differences or would it make sense to de-duplicate that?

I recycled the SNOwGLoBES model class as a parent for both. This model class lives on.

* The `TypeIa` class doesn’t match our `Lastname_YYYY` convention for model classes. Can we rename that to `Wright_2016`? (Which matches the year both preprints appeared on the arXiv; even if the PRD paper on the GCD model is from 2017.)

I am okay with this suggestion. Do you want to keep TypeIa for backwards compatibility?

* I notice the files for PISN and type Ia are in a [combined repo](https://github.com/SNEWS2/snewpy-models-pisn_typeIa/), but the classes are in separate files. I don’t have a strong preference either way; happy to leave it as is, but wanted to flag it in case people see advantages to one or the other. (If joint, maybe `snewpy-models-exotics`/`snewpy.models.exotics` would be a better name, just in case there’s ever any other exotic models.)

I made a single repo so there would be fewer to manage: I'd like to keep the models in separate files.

@JostMigenda

Copy link
Copy Markdown
Member
  • The TypeIa class doesn’t match our Lastname_YYYY convention for model classes. Can we rename that to Wright_2016? (Which matches the year both preprints appeared on the arXiv; even if the PRD paper on the GCD model is from 2017.)

I am okay with this suggestion. Do you want to keep TypeIa for backwards compatibility?

The TypeIa class only ever appeared on this PR branch and was never included in any snewpy release; so I don’t think there is any compatibility issue if we rename it here

@jpkneller jpkneller changed the title PISN & type Ia models PISN & type Ia models - On hold Jul 9, 2026
@jpkneller
jpkneller marked this pull request as draft July 9, 2026 21:45
@jpkneller jpkneller added the on hold Branch is on hold while other branches are being worked onthat need to be merged before this one. label Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on hold Branch is on hold while other branches are being worked onthat need to be merged before this one.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants