-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: master
Are you sure you want to change the base?
Added P92 model #29
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29 +/- ##
==========================================
+ Coverage 99.58% 99.60% +0.02%
==========================================
Files 5 5
Lines 241 255 +14
==========================================
+ Hits 240 254 +14
Misses 1 1 ☔ View full report in Codecov by Sentry. |
@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]>
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.
FIR_lambda = 25.0 | ||
FIR_b = 0.00 | ||
FIR_n = 2.0 | ||
@assert BKG_amp ≥ 0 "`BKG_amp` must be ≥ 0, got $BKG_amp" |
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'm almost wondering if it would be easier to represent this struct hierarchically with tuples or named tuples
struct Example
BKG = (165.0 * (1/3.08 + 1), 0.047, etc...)
# or
FIR = (A=0.012 * (1 / 3.08 + 1), lambda=25.0, b=0.0
end
It's clear this struct has this hierarchical order and it seems organized a little better. The question is whether this is easier for the user or if it makes sense for the user. Perhaps we don't use this for the struct itself, but we add a constructor that allows this behavior, something like
P92(BKG, FUV, NUV, SIL1, SIL2, FIR) = P92(BKG..., FUB..., NUV..., SIL1..., SILL2..., FIR...)
We could make this a kwarg-only version, but we'd have to remove the @with_kw
from the struct (which is fine, we'd just have to add the assertion checking to another constructor, about the same complexity/lines-of-code)
Also, AFAIK this should have no performance impact (tuples and namedtuples are static and have lots of compiler opts.
What do you think?
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.
Your point is correct, that would make the struct quite organized, but let's think from the user's perspective who is trying to switch from DustExtinction (python) to this place, things like this could be intimidating to the user. What do you think?
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.
We can have both. The user isn't necessarily going to interact with the struct directlyy, just with the constructors and the methods. We can organize the struct however we want and organize the constructors however we want. It may be easier to not worry about it
@@ -77,3 +77,172 @@ function (law::FM90)(wave::T) where T | |||
return exvebv | |||
end | |||
|
|||
|
|||
""" | |||
P92(BKG_amp=218.57, |
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.
You'll want to show that these are keyword arguments with the semicolon
P92(; BKG_amp=218.57,
(and propagate the spacing).
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.
Sorry, I clicked on resolved by mistake. 😅
Adding P92 model, we can mention this in #10!