-
Notifications
You must be signed in to change notification settings - Fork 48
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
fix flipped lower and upper in profile CI #785
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #785 +/- ##
=======================================
Coverage 96.99% 97.00%
=======================================
Files 34 34
Lines 3366 3371 +5
=======================================
+ Hits 3265 3270 +5
Misses 101 101
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/profile/fixefpr.jl
Outdated
@@ -102,6 +102,10 @@ function profileβj!( | |||
ζv = getproperty.(tbl, :ζ) | |||
βv = getproperty.(tbl, sym) | |||
val.fwd[sym] = interpolate(βv, ζv, BSplineOrder(4), Natural()) | |||
if sym == :β1 | |||
@debug "" sym | |||
@debug "" collect(zip(ζv, βv)) |
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.
Did you intend to leave this in or is it leftover from debugging?
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.
yes and yes. 😉 I was going to leave it for now in case I get the urge to do a deeper dive in the near future about what's causing the reversal. But I can also strip out it and add it if I ever get around to that.
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 remember seeing an issue quite a while back where having logging macro calls in your code at all came with a performance penalty, even if all uses were in branches that were never executed. I assume it's been fixed in the meantime but don't actually know.
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.
Eeek. That's a good reason to drop it though this isn't a particularly hot section of the code.
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 basically 0 familiarity with this part of the code but what you have here looks reasonable at least. I imagine if there were downstream ramifications then other tests would be affected but that seems not to be the case. So... LGTM
closes #784
docs/NEWS-update.jl
to update the cross-references.