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

doc: add warning to benchmark #90

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Lampese
Copy link
Contributor

@Lampese Lampese commented Jan 10, 2025

Due to moonbitlang/core#1456 I think it needs to be explained in advance that there is a situation where the average time is greater than the maximum time, and according to #66, I mentioned that this is a temporary implementation

Copy link

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are some observations and potential issues from the provided git diff output:

  1. Inconsistent Benchmarking Methodology:

    • The added note in README.md highlights a potential issue with the benchmarking methodology. Specifically, it mentions that the average time might be greater than the maximum time due to measuring time twice (once for average and once for min/max). This inconsistency could lead to misleading results and should be addressed in future updates.
  2. Redundant Type Annotations:

    • In benchmark.mbt, the type annotations for max and min (: Double) were removed. While this might be intentional for code simplification, it could reduce readability for developers who rely on explicit type annotations to understand the code. If the type is clear from the context, this change is fine, but it’s worth ensuring that the code remains maintainable.
  3. Potential Precision Issues:

    • The use of @double.min_value and @double.max_value for initializing min and max might lead to precision issues or unexpected behavior, especially if the actual values being measured are outside the expected range. It’s important to ensure that these initial values are appropriate for the context of the benchmark.

These are the key points to consider based on the provided diff. Let me know if you need further clarification!

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.

1 participant