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

Bug fix in FardalStreamDF. #358

Merged
merged 9 commits into from
Feb 14, 2024
Merged

Bug fix in FardalStreamDF. #358

merged 9 commits into from
Feb 14, 2024

Conversation

jl3937
Copy link

@jl3937 jl3937 commented Feb 1, 2024

Describe your changes

Fix the implementation of FardalStreamDF based on https://arxiv.org/pdf/1410.1861.pdf.
Specifically, we made the changes based on Eq. (21) and Eq. (6) in the paper.

Checklist

  • Did you add tests?
  • Did you add documentation for your changes?
  • Did you reference any relevant issues?
  • Did you add a changelog entry? (see CHANGES.rst)
  • Are the CI tests passing?
  • Is the milestone set?

@adrn
Copy link
Owner

adrn commented Feb 13, 2024

This is a good catch - thanks! I did have a reason for changing these values back when I implemented it, but I can't track down why. It does make sense for the class called FardalStreamDF to be true to the Fardal et al. 2015 publication, as you've fixed, but I'm worried that this will cause some backwards compatibility issues for users who were using it previously. One option would be to add an argument to the class (with a default False value) to let the user specify whether they want to use the original prescription, or the Gala augmented values? Is that something you'd be interested in implementing here?

@jl3937
Copy link
Author

jl3937 commented Feb 13, 2024

This is a good catch - thanks! I did have a reason for changing these values back when I implemented it, but I can't track down why. It does make sense for the class called FardalStreamDF to be true to the Fardal et al. 2015 publication, as you've fixed, but I'm worried that this will cause some backwards compatibility issues for users who were using it previously. One option would be to add an argument to the class (with a default False value) to let the user specify whether they want to use the original prescription, or the Gala augmented values? Is that something you'd be interested in implementing here?

Thanks for reviewing the change! It's a great idea to protect the change by a flag for backwards compatibility. But I guess you could be the better person implementing it in terms of naming, adding description and consistent code style. Just let me know. Thanks!

@adrn
Copy link
Owner

adrn commented Feb 14, 2024

Sure -- I can take it from here. I'll just contribute to this PR, so your contribution is still tracked. Thanks again for raising this!

@adrn adrn added this to the v1.9 milestone Feb 14, 2024
@adrn adrn merged commit ed0bb98 into adrn:main Feb 14, 2024
19 checks passed
@jl3937
Copy link
Author

jl3937 commented Feb 14, 2024

Sure -- I can take it from here. I'll just contribute to this PR, so your contribution is still tracked. Thanks again for raising this!

Thanks for the thorough fixes!

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.

2 participants