new SALT3 model #296
new SALT3 model #296
Conversation
|
The CI failures are for code style and tests. Should be easily fixed. |
|
thanks Ben! Working on this now |
|
this PR goes with sncosmo/sncosmo.github.io#24 for the model files |
There was a problem hiding this comment.
Cool. Can you add the class to the API reference docs in docs/reference.rst?
It looks like there's a fair amount of code duplication with SALT2Model, though I haven't taken a close look at the differences. Any thoughts on how we could reduce code duplication here?
| if (v is not None and isinstance(v, str)): | ||
| names_or_objs[k] = os.path.join(modeldir, v) | ||
|
|
||
| # model components are interpolated to 2nd order |
There was a problem hiding this comment.
I think of "2nd order" as quadratic, but this is using cubic interpolation. Is there a a wording mismatch?
There was a problem hiding this comment.
this is from the original SALT2 - I don't understand it but I'm hesitant to change it, given that it could indicate something about how the model is generated. I removed this from the new code because I realized the only thing I really need to change is how the errors are generated, so I just made SALT3Source an instance of SALT2Source and overwrote the error method.
| self._phase = phase | ||
| self._wave = wave | ||
|
|
||
| # model covariance is interpolated to 1st order |
There was a problem hiding this comment.
"1st order" but using cubic interpolation?
sncosmo/models.py
Outdated
| if version == 0: | ||
| raise Exception("Salt2ExtinctionLaw.version 0 not supported.") | ||
| elif version == 1: | ||
| self._colorlaw = SALT2ColorLaw(colorlaw_range, colorlaw_coeffs) | ||
| else: | ||
| raise Exception('unrecognized Salt2ExtinctionLaw.version: ' + | ||
| version) |
There was a problem hiding this comment.
Minor comment: RuntimeError might be the most appropriate exception here. Also, version = 0 might indicate that the version keyword wasn't found in the file.
There was a problem hiding this comment.
I removed this code from SALT3Source (see comment above), but changed this in the SALT2Source just for fun
There was a problem hiding this comment.
That was probably me who wrote Exception initially! 😄
|
thanks so much for doing this review Kyle, just to summarize what I said above - I realized the only thing I really need to change for SALT3 in sncosmo is how the errors are generated, so I just made SALT3Source an instance of SALT2Source and overwrote the error method in SALT3Source. Seems to work well. |
|
hey @kbarbary , just sending a reminder - think proposed changes have been made. Thanks! |
|
The latest version looks good to me. If there are no objections, I'll merge it on Monday. |
|
thanks so much @kboone - note PR sncosmo/sncosmo.github.io#24 is also necessary for the data files to be included in the repo |
|
Looks good, sorry for the delayed re-review! |
|
no problem, thanks so much for taking the time |
|
well crap, merging the new data repository allowed me to catch my error - SALT3 doesn't require an error scaling file, so it's best to not include a "dummy" file as I'd done in the past. I edited the code to fix the code breaks that happened when the salt3 data were downloaded from the online repository and an "errscalefile" was not found. Assuming this passes checks, my hope is everything will be good to go now. Sorry about that! |
|
I downloaded the final version, and everything seems to work as expected. I'll merge this. |
|
thanks so much! |
No description provided.