Split UCI, into Engine and UCIEngine #5147
Conversation
Disservin
changed the title
Disservin
changed the title
|
|
||
| if (std::abs(v) < VALUE_TB_WIN_IN_MAX_PLY) | ||
| { | ||
| score = InternalUnits{UCIEngine::to_cp(v, pos)}; |
There was a problem hiding this comment.
this does not belong here, as it stands the names are wrong and Score depends on UCI. It should store the value internal units and this conversion should happen in UCIEngine
There was a problem hiding this comment.
Well I'm not sure, I think the to_cp and the wdl shouldn't be a part of the uciengine altogether. If we were to make it a library then the uciengine would be something which the user implements and eval normalization and wdl isn't something they should be doing.
Meaning I think this should be moved to another (new) namespace but still return the normalized value not the internal, or maybe the internal and normalized as a pair?
There was a problem hiding this comment.
Right, I see how this makes sense. It's a bit of a mess because conversion from internal units to cp is no longer straightforward. Not sure how the precision loss would affect people, but it should be minimal. Worst case we could convert to millipawns or smth in the future. So I guess just the naming needs fixing.
src/search.cpp
Outdated
| ss << " nodes " << nodes << " nps " << nodes * 1000 / time << " hashfull " << tt.hashfull() | ||
| << " tbhits " << tbHits << " time " << time << " pv"; | ||
| if (worker.options["UCI_ShowWDL"]) | ||
| info.wdl = UCIEngine::wdl(v, pos); |
There was a problem hiding this comment.
Actually, this is another thing in search that depends on UCIEngine. Could think about removing this. UCIEngine could calculate this itself. Even with more precision as it now has knowledge about TB and mate scores.
If pos is not known but needed it could be placed into info too.
This looks like it could probably be merged for the new capabilities introduced, and iterated over afterwards to refine the interfaces. I'm not expecting such a design to be right from version one, we probably have to understand what is needed to implement different use cases. For example, I believe it would be nice if main would only depend on e.g. uci.h and engine.h, and we'll need to figure out where and how we deal with the internal score to normalized score to wdl conversions.
sure? |
Disservin
force-pushed
the
uci-split-engine
branch
5 times, most recently
from
573fe84 to
e150999
Compare
Disservin
force-pushed
the
uci-split-engine
branch
2 times, most recently
from
748fb24 to
cf62ca0
Compare
|
I've split the pr into two commits, one for the split and one for the callback refactor, I think it makes sense if it is merged without squash. |
This is another refactor which aims to decouple uci from stockfish. A new engine class manages all engine related logic and uci is a "small" wrapper around it. In the future we should also try to remove the need for the Position object in the uci and replace the options with an actual options struct instead of using a map. Also convert the std::string's in the Info structs a string_view. closes official-stockfish#5147 No functional change
Disservin
force-pushed
the
uci-split-engine
branch
from
cf62ca0 to
1689a12
Compare
This is another refactor which aims to decouple uci from stockfish. A new engine class manages all engine related logic and uci is a "small" wrapper around it. In the future we should also try to remove the need for the Position object in the uci and replace the options with an actual options struct instead of using a map. Also convert the std::string's in the Info structs a string_view. closes official-stockfish#5147 No functional change
Disservin
force-pushed
the
uci-split-engine
branch
from
1689a12 to
b9a548d
Compare
Part 2 of the Split UCI into UCIEngine and Engine refactor. This creates function callbacks for search to use when an update should occur. The benching in uci.cpp for example does this to extract the total nodes searched. No functional change
Disservin
force-pushed
the
uci-split-engine
branch
from
b9a548d to
9032c6c
Compare
|
Undrafted and updated with the recent result from fishtest, after the discussion here I also changed some strings to string_views (included in the last fishtest test). |
Disservin
changed the title
This is another refactor which aims to decouple uci from stockfish. A new engine class manages all engine related logic and uci is a "small" wrapper around it.
UCI creates function callbacks for search to use when an update should occur. The benching in uci.cpp for example does this to extract the total nodes searched.
Non Regression STC:
https://tests.stockfishchess.org/tests/view/661016b5bfeb43334bf7d96c
LLR: 2.93 (-2.94,2.94) <-1.75,0.25>
Total: 166368 W: 43110 L: 43033 D: 80225
Ptnml(0-2): 672, 19066, 43650, 19105, 691
In the future we should also try to remove the need for the Position object in the uci and replace the options with an actual options struct instead of using a map.
closes #5147
No functional change