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

Decrease warning levels #20

Merged
merged 3 commits into from
Nov 29, 2022
Merged

Decrease warning levels #20

merged 3 commits into from
Nov 29, 2022

Conversation

tanaya-mankad
Copy link
Contributor

Change WARN to INFO for issues that are handled internally and likely to be expected by the user.

@tanaya-mankad tanaya-mankad self-assigned this Sep 21, 2022
Copy link
Member

@nealkruis nealkruis left a comment

Choose a reason for hiding this comment

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

This looks fine, but I would really like to clean up the warning level so that it is not a global variable. Can we fit that in here?

We should also confirm what happens for cubic interpolations when there are 2 or 3 grid axis values.

@tanaya-mankad
Copy link
Contributor Author

This looks fine, but I would really like to clean up the warning level so that it is not a global variable. Can we fit that in here?

We should also confirm what happens for cubic interpolations when there are 2 or 3 grid axis values.

Do you mean the
extern int LOG_LEVEL? I was perplexed by that one but assumed it must exist for ease of use.

@nealkruis
Copy link
Member

Yeah, that's the one. I think we can come up with a better design.

@tanaya-mankad
Copy link
Contributor Author

As suspected, the cubic interpolation on axes that contain 2 or 3 values just fixes the underspecified point at the same value as the existing endpoint on the relevant side. In other words, if an interpolation happens between index 0 and index 1, the relative coordinates of all the points involved for cubic spline fitting are [-1, 0, 1, 2]. If either points (-1) or (2) are missing, they are fixed to be the same as points (0) or (1) respectively, here. It's currently already tested as well, though I haven't re-hand-calculated this to confirm it's right.

@tanaya-mankad
Copy link
Contributor Author

tanaya-mankad commented Oct 31, 2022

As for LOG_LEVEL, its entire existence feels superfluous to me - it adds unnecessary complexity to what should be handled straight up by an error callback.
By the same token though, the error callback is also global, and can be overwritten by any piece of code in the same project that has access to the Btwxt namespace...
[edit] @nealkruis What do you think about having a callback object per btwxt instance? The global callback led to some design workarounds in E+ that I wasn't too fond of...

@nealkruis
Copy link
Member

@tanaya-bigladder do we want to work on this, or just follow up with the callback change?

@tanaya-mankad
Copy link
Contributor Author

@nealkruis yes, let's punt. I discovered yesterday that it's a can of worms that will be best with some serious refactoring. I know what we should do but we ought to discuss first.

@nealkruis nealkruis merged commit 7fe17b5 into develop Nov 29, 2022
@nealkruis nealkruis deleted the decrease-warning-levels branch November 29, 2022 20:10
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