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

fixing inconsistent legendre wavelength basis (#291) #299

Merged
merged 9 commits into from
Apr 25, 2024
Merged

Conversation

abhi0395
Copy link
Member

@abhi0395 abhi0395 commented Apr 24, 2024

Thanks @sbailey for pointing me to the issue of inconsistent legendre wavelength basis definition in code. The issue is detailed in issue#291 have now defined a separate utility function reduced_wavelength() in utils.py, which is called whenever we want to define legendre wavelength basis. It makes the code cleaner and consistent in archetype mode. This consistent definition should also be included in PR#293 and PR#283.

It also fixes a minor case, when an end user does not want to add a prior in the archetype mode. I have implemented a minor logic, that if --archetype-legendre-prior flag is <=0, then no prior will be added in archetype mode.

@coveralls
Copy link

coveralls commented Apr 24, 2024

Coverage Status

coverage: 38.666% (-0.7%) from 39.333%
when pulling cc3c2f4 on correct_legendre
into 2ee8544 on main.

@sbailey
Copy link
Collaborator

sbailey commented Apr 25, 2024

Thanks. For the record, this PR also updates the wavelength -> [-1,1] mapping to be per-camera instead of using the full wavelength basis across all cameras. I think that is what we want so that the legendre polynomials are orthogonal per camera.

I also added unit tests on reduced_wavelength which caught a wavemax vs. wavemin bug which I corrected.

@sbailey sbailey merged commit 9428cf9 into main Apr 25, 2024
12 checks passed
@sbailey sbailey deleted the correct_legendre branch April 25, 2024 00:10
@abhi0395
Copy link
Member Author

Thank you so much. Luckily, I hadn't run anything big yet.

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.

3 participants