Save results #75
Save results #75
Conversation
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
==========================================
- Coverage 46.13% 46.00% -0.14%
==========================================
Files 18 18
Lines 2265 2276 +11
==========================================
+ Hits 1045 1047 +2
- Misses 1220 1229 +9
Continue to review full report at Codecov.
|
measure_extinction/extdata.py
Outdated
| # save the column values if available | ||
| if "AV" in self.columns.keys(): | ||
| hname.append("AV") | ||
| hcomment.append("V-band extinction A(V)") | ||
| hval.append(self.columns["AV"]) | ||
| if "RV" in self.columns.keys(): | ||
| hname.append("RV") | ||
| hcomment.append("total-to-selective extintion R(V)") | ||
| hval.append(self.columns["RV"]) |
There was a problem hiding this comment.
Isn't this done in the code right above? Maybe not, but it seems to be very similar. Maybe the column_info dictionary needs to be updated/populated/created if Av or Rv (or other column info) is in the self.columns dictionary?
There was a problem hiding this comment.
I was confused about that myself. I saw the code just above with "column_info", but I was not sure what it is doing, and I didn't want to break anything, so just to be sure I added my own code, and knew you would complain if that was not the way to go :) I didn't fully understand what that part of the code is doing. It looks like it is saving what's in column_info, but column_info needs to be given as an argument with the save function. Is that how you are using the code? I.e. do you store the calculated AV and RV somewhere and then give it to the save function: extdata.save(column_info={blabla})? I directly store the calculated AV and RV in extdata.columns and then with this added code make sure extdata.columns is saved to the fits. If I have to give it as an argument to the save function, then the extdata.columns is kinda useless... Sorry if this is not very clear.
There was a problem hiding this comment.
I am wondering if a way forward would be to test if "AV"/"RV" are in the column_info dictionary. If they are, then do nothing and have the code use the column_info dict info to populate the fits header. If they are not present, then add the "AV"/"RV" info to the column_info dict (create it if needed) - this would use the existing column_info code to populate the fits header. This makes it clear what happens if both are specified and reduces duplicating capabilities in the code.
There was a problem hiding this comment.
Ok, I can adjust the code in that way. I am not sure yet what to do with the uncertainties though. The EMCEE fit gives a minus and plus uncertainty, but the code is only accepting 1 uncertainty at this point. We can have a quick discussion on Wednesday about what conventions to use for uncertainties in general, and I will update the code after that.
There was a problem hiding this comment.
Great.
Yeah, I agree the way these values are handled isn't ideal. I'm thinking we should switch to saving them as a table in an extension instead of as header keywords (maybe keep a few as FITS header keywords as well?). Having the parameters in a table would allow for easy expansion and a much more compact form. I'll see about opening an issue for this.
There was a problem hiding this comment.
I fixed the code so that it is clear what happens if both column_info and self.columns are populated. It will give preference to the information given in the column_info argument. This is a temporary work-around, and I agree that a table would be a better way to store all this information, as explained in issue #77.
When saving an extinction curve to a FITS file, it is now also saving the R(V) and A(V) values, and the uncertainties on the fitted parameters.