Skip to content
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

LMC Bar method added #50

Merged
merged 13 commits into from
Feb 13, 2025
Merged

Conversation

stefanocovino
Copy link
Contributor

Dear friends,

I've added, some time ago, the LMC average model in Gordon et al. (2003) to the list of the available recipes. Might it be of interest for the main branch?

Stefano

stefanocovino and others added 8 commits March 4, 2024 16:39
Added G03_LMCAve reference.
Added data for G03_LMCAve law.
Added tests for G=£_LMCAve law.
The code works, I am using it. Unfortunately, I could not solve the various test errors from @inferred. I wonder whether they might depend on the very old Unitful and UnitfulAstro package versions listed in the Project.toml file.
Copy link
Member

@cgarling cgarling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Just a few comments regarding tests and domain errors.

test/mixture_laws.jl Outdated Show resolved Hide resolved
src/mixture_laws.jl Outdated Show resolved Hide resolved
src/mixture_laws.jl Show resolved Hide resolved
@stefanocovino
Copy link
Contributor Author

I see there were several updates in the main branch I did not include. My fault, of course. As a matter of fact, this fork was ready since several months, but I never manage to pull a request for a long story (I was in contact with one of the developers of this package that, I guess, eventually left the role but I was waiting for comments by him). Anyway, I did not change any part of the code apart from adding the g03lmc recipe.

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (cc36486) to head (8c3ac7d).
Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   99.61%   99.62%   +0.01%     
==========================================
  Files          10       10              
  Lines         261      270       +9     
==========================================
+ Hits          260      269       +9     
  Misses          1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cgarling
Copy link
Member

Fixes #44 and lgtm

@icweaver
Copy link
Member

Thank you for this all! Should some docs and a plot also be added, similar to past models?

@stefanocovino
Copy link
Contributor Author

Hi,

I have thought about this. However, the LMC extinction curve I've added is from a paper (Gordon et al. 2003) already considered in the package. And there are no plots based on these recipes. So, I did not prepare any plot. Nevertheless, if needed, it would be easy to prepare some.

@cgarling
Copy link
Member

I pushed a commit to add the (short) docstring to the @docs block but idk about the plot -- it looks like they are checked into the repo but I usually prefer to generate them on-the-fly during the CI run so that the git history tracks the code that generated the plots rather than the image files themselves. I also find plots overlaying different laws at fixed R_V more useful than having one plot per law showing multiple R_V values, but that's a personal preference.

I would be happy to merge as-is and worry about the plot at a later time.

@stefanocovino
Copy link
Contributor Author

I agree.

@icweaver
Copy link
Member

icweaver commented Feb 13, 2025

Alrighty, let's get it y'all. Thanks @stefanocovino for this really nice PR!

Oh yea, did we want to squash any commits? Totally fine either way, just getting back up to speed on what maintenance things are preferred here

@cgarling cgarling merged commit af42144 into JuliaAstro:master Feb 13, 2025
10 checks passed
@cgarling
Copy link
Member

Sorry I didn't see your edit -- I don't usually worry about squashing unless I know there's a lot of junk commits that I don't think have value (i.e. I did something wrong and just pushed a new commit rather than reverting).

This was referenced Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants