Make use of slog a feature
#136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stefan-k
requested changes
Sep 8, 2021
|
Thanks a lot for your contribution and for the fruitful discussion! :) Let me know when you think it is ready to go and I will merge the PR. |
There were a few errors to take care of here: 1. In `sr1_trustregion.rs`, there was an unnecessary borrow. 2. In many cases `assert_eq!(..., true)` or `assert_eq!(..., false)` were being used. This is easily replaced by doing `assert!(...)`. 3. There were quite a number of checks on floating point numbers in `iterstate.rs` that related to determining if a number was finite or not. These are easily replaced by checking `value.is_infinite()`, along with changing the `assert_eq!` to `assert!`. 4. There were a number of checks in the tests for the cost function that compared two floating point values directly. This is always wrong. For byte equality, it should compare `state.get_cost().to_ne_bytes()` to `cost.to_ne_bytes()`. However, here the comparison is to validate that these are in fact the same number. As the crate already depends on `approx`, it is better to use `assert_relative_eq!` and set an epsilon. Here I chose `f64::EPSILON`, which I would normally argue is a bad choice, because it does not respect significance. However, I'm just fixing lints, so I leave better choices of epsilon values to the official maintainers.
The flag is set as a default feature. If users don't want slog, now they can turn it off by doing: ```TOML [dependencies.argmin] version = "0.4" default-features = false features = ["nalgebral", "ndarrayl"] ``` And they won't need to worry about building SLOG or its dependencies. This is especially useful if you want to use a different logger (e.g. env_logger), but argmin carries in slog as a transient dependency.
ThatGeoGuy
force-pushed
the
slog-as-feature
branch
from
f783b9b
to
1f36efe
Compare
|
Apologies, I saw this and completely forgot about it (oof). I've force pushed, should be good to merge now. |
|
No problem! Thanks again! :) |
stefan-k
approved these changes
Sep 22, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See the two commits provided for more context.
The main crux of this is that I want to use
argmin, but I don't want to includeslogin the process. It makes sense then to make it a feature. Given how prolific this feature is throughout the library (it is the default terminal logger, even the file logger just goes to bincode / JSON), I opted to leave it as a default feature. Of course, if users don't wantslogthemselves, then they just have to specify the following in theirCargo.toml:With whatever features they do want enabled explicitly in the
dependencies.argmin.featuresarray.The first commit just cleaned up a bunch of clippy lints. The command I'm using is:
I'm also on clippy (0.1.54). Hopefully the reasoning in the commit message tracks.