official-pikafish / Pikafish

UCI xiangqi engine
http://pikafish.org
GNU General Public License v3.0
867 stars 162 forks source link

Not avoiding the possibility of -∞ value in UCI::pv #45

Closed ChessOverflow closed 1 year ago

ChessOverflow commented 1 year ago

The -∞ value only occurs when there is a bug in the search function or the evaluation function, so changing it to zero value is completely wrong, and this will prevent from Solving the bugs.

PikaCat-OuO commented 1 year ago

Thanks ahead. But if you observe those commits on Pikafish, you will find that they all come from Stockfish except for Xiangqi-specific implementation. So here is the deal, for non-Xiangqi-related code, PR to Stockfish and make them merge it. If it's successfully merged in Stockfish, then I'll cherry-pick them to Pikafish instead of a direct change on Pikafish. When cherry-picking, the author's information will remain.

ChessOverflow commented 1 year ago

Thanks ahead. But if you observe those commits on Pikafish, you will find that they all come from Stockfish except for Xiangqi-specific implementation.

I can't perform any action on stockfish repo. Due to the https://github.com/official-stockfish/Stockfish/discussions/4678, Admins restricted me there.

Please make a PR to Stockfish and make them merge. Please note that not merging this PR can cause many bugs for evaluation or search function in the future.

good luck

PikaCat-OuO commented 1 year ago

Thank you for your suggestion for bringing this to my attention. However, I typically do not submit PRs for ideas that aren't originally mine. It's just a personal principle I adhere to. I'd recommend reaching out to someone else or exploring other avenues to address this issue. I truly appreciate your understanding and support on this matter. Note that If this is properly addressed in Stockfish and get merged, I'll cherry-pick that commit here right away. Best wishes.

ChessOverflow commented 1 year ago

I'd recommend reaching out to someone else or exploring other avenues to address this issue.

I've created this PR to someone else but he edited it and replaced it with nonsense text. See this:

I really can't think of any other way, please create a PR in Stockfish reposity.

PikaCat-OuO commented 1 year ago

I additionally investigated this line on git blame, https://github.com/official-stockfish/Stockfish/issues/2757 So there is a reason for this line to be in its place. It's going to output "mate 0" w/o this line if the first iteration is not finished and we are required to output things according to UCI protocol. If u want to make it better, then go there or discord to suggest things.

ChessOverflow commented 1 year ago

I investigated this PR https://github.com/official-stockfish/Stockfish/pull/2764.

Maybe they disable/comment this code to troubleshoot the program.

Is search.cpp and evaluate.cpp for Pikafish exactly the same Stockfish?

If it is different, it will definitely cause problems.

PikaCat-OuO commented 1 year ago

Is search.cpp and evaluate.cpp for Pikafish exactly the same Stockfish?

Yes, all the logic is totally the same, except for some values in those formulas that are tuned to better match Xiangqi. I can give you an example of this.

Pikafish:

  // Futility margin
  Value futility_margin(Depth d, bool noTtCutNode, bool improving) {
    return Value((119 - 34 * noTtCutNode) * (d - improving));
  }

Stockfish:

  // Futility margin
  Value futility_margin(Depth d, bool noTtCutNode, bool improving) {
    return Value((140 - 40 * noTtCutNode) * (d - improving));
  }
ChessOverflow commented 1 year ago

These minor changes will not cause errors. Considering the removal of classical evaluation, there is no difference in the evaluate.cpp. I close this PR but note that many of the Stockfish commits are wrong and copying them will be wrong. Good luck.

PikaCat-OuO commented 1 year ago

As a fork instead of a pure repo, we will keep up to date with every commits of upstream Stockfish, regardless of right or wrong, true or false.