-
Notifications
You must be signed in to change notification settings - Fork 8
Added P92 model #29
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
Added P92 model #29
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #29 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 11
Lines 316 347 +31
=========================================
+ Hits 316 347 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mileslucas @giordano it would be really nice if you can review this! |
|
Also, you are missing an entry in the table in |
Co-authored-by: Miles Lucas <[email protected]>
mileslucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some comments about whether we should restructure the P92 struct, so if we go with that there'll be some changes, but otherwise this looks good to go if you bump the minor version and fix that docstring.
|
Will be back for you ='( |
This is mostly to simplify the diff for PR #29
This is mostly to simplify the diff for PR #29
This is mostly to simplify the diff for PR JuliaAstro#29
|
Brought this PR up-to-date with the newer interface and implementation of DustExtinction.jl. Once the CI checks pass, this should be good to go! (Probably do a squash merge because wow the commit tree in |
icweaver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up, Abhro! I've just left my minor formatting suggested edits + one science question
Co-authored-by: Ian Weaver <[email protected]>
Co-authored-by: Ian Weaver <[email protected]>
Co-authored-by: Chris Garling <[email protected]>
icweaver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following up from #29 (comment), tests look to be passing with the value from the paper. I tagged this in this current round of reviews. Otherwise this lgtm, thanks again for wrapping up this longstanding PR!
Co-authored-by: Ian Weaver <[email protected]>
|
Hmm. The commit a93b178 seems to break the doctest, but I'm not sure where the original expected doctest output came from and how to validate it. |
|
Right, that was just a partial suggestion in my above comment. The new value still needs to be updated in the docstring to match. Will take a look when back bu computer to see if anything deeper is going on |
|
Interesting. It looks like Karl specifically changed the default from 0.08 to 0.07 back in 2018 as part of karllark/dust_extinction#50 I see he mentioned that an analytic tests was not being satisfied by this new change, and there looks to be another model fitting issue for P92 open since 2022. I haven't done any deeper digging, but I wonder if this might be related. At any rate, I'm pushing the remaining changes back to the paper value of 0.08 now. I don't think there was anything particularly special about the doctest values that Siddharth used for the start of this PR |
|
I'll also leave a ping in that repo to follow up |
|
@icweaver Is this good to merge? |
|
Absolutely. Thanks for these really nice changes! |
Update: The value has been changed back to 0.08 in karllark/dust_extinction#266 as well, so I think we're all good here |
Adding P92 model, we can mention this in #10!