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/b value estimation #204

Merged
merged 9 commits into from
Feb 18, 2025
Merged

Bug/b value estimation #204

merged 9 commits into from
Feb 18, 2025

Conversation

aronsho
Copy link
Collaborator

@aronsho aronsho commented Feb 14, 2025

I worked on two issues here:

  1. The a/b-value estimators gave an incorrect error
    -> when there was no magnitudes above mc, it gave back a binning error, this is fixed now.
    -> While I was on it, I also included an error if there were nan values
    -> Also, I changed the order: first sanity check, then filtering

  2. I included the possibility of including a time vector for the b-positive methods
    -> Now, the magnitudes do not have to be given sorted if the time is given
    -> While on it, I included the capability of including the index as an attribute of the class. Now, it is very easy to retrieve the actual magnitudes used: magnitudes[estimator.idx]. This is super useful also, when there is some other array that we want to filter in the same way as the magnitude differences are.

@lmizrahi : Would be cool if you used the estimator and checked if it works correctly
@schmidni : I messed a bit with the estimator classes, so I thought it would be good to have you loook at it :)

@aronsho aronsho requested review from lmizrahi and schmidni February 14, 2025 09:48
Copy link
Member

@schmidni schmidni left a comment

Choose a reason for hiding this comment

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

some coments I just made in one place, but should also be applied in other places (like when you did the same changes in a and bvalue estimators, or error types and so on.

@aronsho aronsho force-pushed the bug/b_value_estimation branch from b534803 to 187f307 Compare February 14, 2025 14:17
@aronsho aronsho force-pushed the bug/b_value_estimation branch 2 times, most recently from a13bbf8 to 59e77a2 Compare February 15, 2025 11:07
@aronsho
Copy link
Collaborator Author

aronsho commented Feb 18, 2025

Ok, I believe I addressed all the points, hope that we can merge this one, too! :)

@aronsho aronsho requested a review from schmidni February 18, 2025 08:00
Copy link
Member

@schmidni schmidni 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 your patience :) great work!

@aronsho aronsho merged commit 2caafaf into main Feb 18, 2025
2 checks passed
@aronsho aronsho deleted the bug/b_value_estimation branch February 18, 2025 11:06
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