-
Notifications
You must be signed in to change notification settings - Fork 71
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
adding phase dependence to prop. effects #359
Conversation
It looks like there are some errors in the testing. I know some are on SNCosmo's side but @jpierel14 does the p39 tests pass locally? |
ci tests
Justin and I talked a bit offline. We noted that making phase be the first parameter (though logical if we were just starting) breaks backwards compatibility (see failing tests). Likely it is best to make phase an optional argument rather than a required positional argument. |
@jpierel14 do you know why To get this merged, I think we should
|
The code style error is an easy fix, and I will do that. Let me also review the changes to look for maintainability problems. Other than that, it looks like we are on the right path to adding this new feature. |
@benjaminrose Okay new try |
Look at all those check marks |
I have a few documentation additions. I was going to do them myself, but I con't have push access to your repo. I'll just push them to main after I merge this in. |
Sounds good, thanks a lot! |
@benjaminrose PR to add phase dependence to propagation effects. I don't think this should cause any issues (I've tested phase-dependent, wavelength-dependent, and phase+wavelength-dependent effects), but happy to discuss.