-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add MLCS2k2 model #113
Add MLCS2k2 model #113
Conversation
Whitespace and auto-loader
@kbarbary I think leaving the transpose in the might be okay. The problem seems to be that the FITS standard favors a column-major format. Since that FITS file might be used elsewhere, I feel like it should follow the FITS convention, and environments which use the opposite convention should be responsible for doing the transpose (e.g. Python). If I rewrote the file the other way around, someone's IDL code would then need to do the transpose... But I guess I could be convinced to do it. Speaking of that file, I realized I used the wrong set of MLCS vectors to create it. I've updated it based on a better set (which is also the "default" MLCS2k2.v007 model in SNANA). If you have cached the mlcs2k2.modelflux.fits file, you should delete it and have it re-downloaded. Maybe I should rename it to something more informative about which set of vectors it is? And we can have multiple versions in the url json file? Right now we haven't included the model errors in any way. I should do this in the same way it's done for SALT in sncosmo, but that will take some time, so it probably does make sense to merge now and have the model included. Finally, I updated the demo notebook to work with the current way it is set up: |
I agree that the file should follow the FITS convention and that Python should adapt as needed. For example, this is what is done with images: In FITS, the "x" axis is NAXIS1 and the "y" axis is NAXIS2. In Python, we follow the FITS convention of the "x" axis being the axis contiguous in memory, so we simply index images like For this file, regardless of what is done in Python, it seems like wavelength should be the axis contigous in memory (NAXIS1), phase should be next (NAXIS2) and delta should be the slowest index (NAXIS3). I don't have a really good reason for this, except that one most often wants to access a 1-d spectrum slice of the array, which would then be a contiguous block of memory. I'm aware of a FITS convention that "x" is NAXIS1 in images. Is there a similar convention for arrays like this that mix various dimensions (wavelength, time, other)? |
The Travis failure is due to Yet another alternative that I thought of recently would be to instantiate a |
I agree with this, and didn’t realize that NAXIS1 is contiguous in memory. I thought it was the other way around. Yes, we definitely want the wavelength axis contiguous. Okay, so I’ll update the code that creates the FITS file and reverse it, and update the read3dgrid routine.
|
sounds good! |
Any progress on this, @saurabhwjha? |
I've made a new FITS file (you will need to delete .astropy/cache/sncosmo/models/mlcs2k2/mlcs2k2.modelflux.fits and have it download the new version). This flips the order of the axes, and also is now 32 bit floating point instead of 64 bit (which was overkill) so that makes it half the size. I've updated the read_3dgriddata_fits function to deal with this; no transpose anymore. These are all committed on my fork: saurabhwjha@5ad0e4e |
Great. One remaining item: do you want to put some sort of version number in the file name? That will make any future bug fixes easier for users: They won't have to manually delete anything. For the SALT2 model for example, v1.0 was the original, v1.1 was a bug fix. |
Okay, I've created a v1.0 MLCS2k2 model. I've added some HISTORY keywords in the FITS header to specify which MLCS model this is. In MLCS2k2 language, this SED corresponds to "MLCS2k2 v0.07 rv19-early-smix vectors". The download link for v1.0 is: The old file (without the v1.0) has been removed, so please update the urls.json file. |
I changed the urls.json file and made a PR on your branch that should fix failing tests. |
Generalize read_griddata_fits
Travis failures are some unrelated problem with the conda path. Merging this. Thanks Saurabh! |
This could use some testing and docs, but I think we can merge it now.
I'd also like to avoid the transpose in
read_3dgriddata_fits
and possibly combine the function withread_griddata_fits
, but again, we can look at that after merging.cc @saurabhwjha