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

API: synchonisation iode-dos 6.70 api -> iode 7.0 api #373

Merged
merged 20 commits into from
Oct 17, 2023
Merged

Conversation

jmpplan
Copy link
Collaborator

@jmpplan jmpplan commented Oct 12, 2023

Synchonisation iode-dos 6.70 api -> iode 7.0 api

Reports functions

  • new command $silent
  • new function @version()
  • command $msg: bug fixed
  • new @functions related to simulations:
    @SimSortNbPasses()  Last "nbtri" parameter
    @SimSortAlgo()      Last sort parameter: "Connex", "Triangulation" or "None"
    @SimInitValues()    Last starting values parameter
    @SimCpuScc()        Time in ms spent to compute Connex Components
    @SimCpuSort()       Time in ms spent to reorder the interdependent block of the model
    @SimCpu(period)     Time in ms elapsed during last simulation of the specified period
    @SimNorm(period)    Convergence criterion reached for the given period during last simulation
    @SimNIter(period    Number of iteration needed to reach a solution for the given period during last simulation

Endo-Exo bug fixed

In a simulation, when an exchange endo-exo is required, if one of the vars does not exist,
an error message is now provided. In the previous versions, IODE crashed if that was the case.

-> Fix #330

Simulation algorithm

The method for decomposing models into strongly connex components has been revised and greatly improved
for large models.

FileImportVar and FileImportCmt

-> Fix #292.

@alixdamman
Copy link
Collaborator

alixdamman commented Oct 12, 2023

@jmpplan please note that both unittests

	 47 - SimulationTest.Simulation (Failed)
	 49 - SimulationTest.SimulateSCC (Failed)

failed. See Github Actions results here

Both unittests seems linked to the C++ API. So I guess some of your changes broken the C++ API in some manner.

Please compile the C++ API and the associated tests on your PC to solve the problem.

@alixdamman
Copy link
Collaborator

I have noticed I broke the test_sanitize job from the github-actions-build-and-test.yml file.

Please execute the following lines in a terminal:

> git fetch --all
> git pull --rebase origin master
> git push --force

@jmpplan
Copy link
Collaborator Author

jmpplan commented Oct 12, 2023

@jmpplan please note that both unittests

	 47 - SimulationTest.Simulation (Failed)
	 49 - SimulationTest.SimulateSCC (Failed)

failed. See Github Actions results here

Both unittests seems linked to the C++ API. So I guess some of your changes broken the C++ API in some manner.

Please compile the C++ API and the associated tests on your PC to solve the problem.

I've modified test1.c to correct the simulation errors found by GH, but test_c_api.c does not compile anymore, even on my computer! Reason: the flag -std:c++20 FOLLOWS the flag /Zc:strictStrings-.
I've made some tests locally with nmake and cl.exe and I've found that if the parameter positions are inverted, it works, indicating that the last parameter has the priority.
Another solution that works is to use -std:c++17 instead of -std:c++20. BUT the method str.starts_with() does not exist in std lib C++17 ☹

Now, if we want to compile with -std:c++20, there is a lot of work to do. Only in test_c_api.c, cl finds more than 100 errors! Often when calling the same functions, like K_add() but still...

@alixdamman
Copy link
Collaborator

alixdamman commented Oct 13, 2023

Now, if we want to compile with -std:c++20, there is a lot of work to do

Why ? It is compiling on both my PC (MSVC 1935 -> I've installed Visual Studio 2022 some months ago) and on GH (still running on Windows 2019 with MSVC < 1929).

On GH, two tests failed (from folder tests/test_cpp_api, not tests/test_c_api !):

 47 - SimulationTest.Simulation (Failed)
49 - SimulationTest.SimulateSCC (Failed)

Concerning SimulationTest.Simulation, please change the line 90 of the file tests/test_cpp_api/test_simulation.cpp (unittest Simulation) by

    EXPECT_DOUBLE_EQ(round(Variables.get_var("XNATY", "2000Y1") * 10e5) / 10e5, 0.800703);

Concerning SimulationTest.SimulateSCC, the logs on GH Actions say:

Running main() from D:\a\iode\iode\out\build\windows-debug\_deps\googletest-src\googletest\src\gtest_main.cc
Note: Google Test filter = SimulationTest.SimulateSCC
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from SimulationTest
[ RUN      ] SimulationTest.SimulateSCC
Loading D:\a\iode\iode\out\build\windows-debug\tests\data\fun.eqs
274 objects loaded
Loading D:\a\iode\iode\out\build\windows-debug\tests\data\fun.scl
161 objects loaded
Loading D:\a\iode\iode\out\build\windows-debug\tests\data\fun.var
394 objects loaded
Pseudo-linking equations ....
Calculating SCC...
Calculating SCC... 0 ms -> #PRE 31 - #INTER 204 - #POST 39
Reordering interdependent block...
Reordering interdependent block... 0 ms
Linking equations ....
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
[  FAILED  ] SimulationTest.SimulateSCC (12 ms)
[----------] 1 test from SimulationTest (12 ms total)

I don't know the reason of the SEH exception error but I made a checkout of your branch on my PC (MSVC 1935 - Visual Studio 2022) and I have no error for this unittest. So I suggest that I update the GH Actions runner to Windows server 2022 (with Visual Studio 2022)

@alixdamman
Copy link
Collaborator

Mmm... could you run the unittest SimulateSCC (from test_simulation.cpp) in a debug mode to try to see where the problem comes from ?

@jmpplan
Copy link
Collaborator Author

jmpplan commented Oct 13, 2023

Now, if we want to compile with -std:c++20, there is a lot of work to do

Why ? It is compiling on both my PC (MSVC 1935 -> I've installed Visual Studio 2022 some months ago) and on GH (still running on Windows 2019 with MSVC < 1929).

I mean if we want to remove the flag /Zc:strictStrings-, all the static strings have to be replaced and/or some function signatures must be modified (K_add() for example) to add the const keyword when appropriate.

On GH, two tests failed (from folder tests/test_cpp_api, not tests/test_c_api !):

 47 - SimulationTest.Simulation (Failed)
49 - SimulationTest.SimulateSCC (Failed)

Concerning SimulationTest.Simulation, please change the line 90 of the file tests/test_cpp_api/test_simulation.cpp (unittest Simulation) by

    EXPECT_DOUBLE_EQ(round(Variables.get_var("XNATY", "2000Y1") * 10e5) / 10e5, 0.800703);

Done. But I had to change it also in test_c_api (with a slightly different value). I don't understand where these differences are coming from ??? I should add a new issue.

Concerning SimulationTest.SimulateSCC, the logs on GH Actions say:

Running main() from D:\a\iode\iode\out\build\windows-debug\_deps\googletest-src\googletest\src\gtest_main.cc
Note: Google Test filter = SimulationTest.SimulateSCC
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from SimulationTest
[ RUN      ] SimulationTest.SimulateSCC
Loading D:\a\iode\iode\out\build\windows-debug\tests\data\fun.eqs
274 objects loaded
Loading D:\a\iode\iode\out\build\windows-debug\tests\data\fun.scl
161 objects loaded
Loading D:\a\iode\iode\out\build\windows-debug\tests\data\fun.var
394 objects loaded
Pseudo-linking equations ....
Calculating SCC...
Calculating SCC... 0 ms -> #PRE 31 - #INTER 204 - #POST 39
Reordering interdependent block...
Reordering interdependent block... 0 ms
Linking equations ....
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.
[  FAILED  ] SimulationTest.SimulateSCC (12 ms)
[----------] 1 test from SimulationTest (12 ms total)

I don't know the reason of the SEH exception error but I made a checkout of your branch on my PC (MSVC 1935 - Visual Studio 2022) and I have no error for this unittest. So I suggest that I update the GH Actions runner to Windows server 2022 (with Visual Studio 2022)

In previous version we already had similar errors. The problem came from the modification of static strings (like "" passed as parameter and modified inside the function). To solve that problem, we can either add the const keyword where appropriate, or replaced static data by non static data:

char empty_string[1] ;
...
call to Fn(""...) 
replaced by Fn(empty_string...)

Or add the keyword /Zc:strictStrings-, but it does not work in all configurations on my pc...

@alixdamman
Copy link
Collaborator

Done. But I had to change it also in test_c_api (with a slightly different value). I don't understand where these differences are coming from ??? I should add a new issue.

Good idea. As I understand it, you didn't expect to have those differences given the changes you made in the C API code.

@alixdamman
Copy link
Collaborator

I mean if we want to remove the flag /Zc:strictStrings-, all the static strings have to be replaced and/or some function signatures must be modified (K_add() for example) to add the const keyword when appropriate.

That would be the cleanest way. But that could be an important work...
If you want to go this way, please do it in a separate branch and PR.

@jmpplan
Copy link
Collaborator Author

jmpplan commented Oct 13, 2023

Mmm... could you run the unittest SimulateSCC (from test_simulation.cpp) in a debug mode to try to see where the problem comes from ?

I already tried it before but for a mysterious reason, it does not compile: for example:

C:\PROGRA~2\MICROS~4\2019\COMMUN~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe  /nologo /TP -DDEBUG=1 -DDOS -DDOSW32 -DIODEWIN -DNOEMF -DNOEMS -DREALD -DSCRPROTO -DVC -DWINDOWS -DWINDOWS_IGNORE_PACKING_MISMATCH -I..\..\..\ -IC:\local\boost_1_74_0 -I..\..\..\cpp_api -I..\..\..\scr4\. /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1 -MDd -D_CRT_SECURE_NO_WARNINGS /Zc:strictStrings- -Zi -std:c++latest /showIncludes /Focpp_api\CMakeFiles\iode_cpp_api.dir\KDB\kdb_global.cpp.obj /Fdcpp_api\CMakeFiles\iode_cpp_api.dir\iode_cpp_api.pdb /FS -c ..\..\..\cpp_api\KDB\kdb_global.cpp
C:\Users\jmp\source\repos\iode\cpp_api\KDB\kdb_global.cpp(29): error C2664: 'int B_WsClear(char *,int)': cannot convert argument 1 from 'const char [1]' to 'char *'
  ..\..\..\cpp_api\KDB\kdb_global.cpp(29): note: Conversion from string literal loses const qualifier (see /Zc:strictStrings)
  C:\Users\jmp\source\repos\iode\api\iodebase.h(1219): note: see declaration of 'B_WsClear'

As you can see, CMake adds the parameter std:c++latest (which does not exist according to the cl help) AFTER /Zc:strictStrings- even though I've moved the 2 blocks in my version of the main CMakeLists.txt;

# some code in the C++ API requires C++ 20
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

if(MSVC)
  add_compile_options("/Zc:strictStrings-")
endif()

@alixdamman
Copy link
Collaborator

I don't know the reason of the SEH exception error but I made a checkout of your branch on my PC (MSVC 1935 - Visual Studio 2022) and I have no error for this unittest. So I suggest that I update the GH Actions runner to Windows server 2022 (with Visual Studio 2022)

In previous version we already had similar errors. The problem came from the modification of static strings (like "" passed as parameter and modified inside the function). To solve that problem, we can either add the const keyword where appropriate, or replaced static data by non static data:

char empty_string[1] ;
...
call to Fn(""...) 
replaced by Fn(empty_string...)

If it is not too much work, you could try this.

@jmpplan
Copy link
Collaborator Author

jmpplan commented Oct 13, 2023

Done. But I had to change it also in test_c_api (with a slightly different value). I don't understand where these differences are coming from ??? I should add a new issue.

Good idea. As I understand it, you didn't expect to have those differences given the changes you made in the C API code.

Exactly. Worrying.

@alixdamman
Copy link
Collaborator

As you can see, CMake adds the parameter std:c++latest (which does not exist according to the cl help) AFTER /Zc:strictStrings- even though I've moved the 2 blocks in my version of the main CMakeLists.txt;

In this case, that could be because me/GH and you are not using the same version of CMake. I don't know.

    - removed unnecessary comments and commented lines
    - some comments translated into En to avoid accents
    - extended comments for simulations variables
    - added new sim variables declarations: KSIM_CPUS, KSIM_CPU_SCC, KSIM_POSXK_REV and KSIM_CPU_SORT
    - declarations of new functions for Cython implementation:
        - IodeModelCalcSCC(int nbtris, char* pre_listname, char* inter_listname, char* post_listname, char *eqs_list)
        - int IodeModelSimulateSCC(char *per_from, char *per_to, char *pre_eqlist, char *inter_eqlist, char* post_eqlist, double eps, double relax, int maxit, int init_values, int debug, double newton_eps, int newton_maxit, int newton_debug)
        - double IodeModelSimNorm(char* period)
        - int IodeModelSimNIter(char* period)
        - int IodeModelSimCpu(char* period))
    - L_link1_endos(): for exogenous vars, cl->lnames[i].pos = -1 instead of KNB(dbe). Needed for the new model reordering algo
    - T_insert_line(): description of parameter 'where' modified
    - B_CreateVarFromVecOfInts(): if the variable cannot be created (bad name...), an error message is provided
    - B_FileImportCmt() and B_FileImportVar(): in call to IMP_RuleImport(), "" replaced by a local empty string
    to avoid crash in SCR_strip()
    - RP_silent(char* arg): implements $silent
    - RP_beep(): added char* parameter
    - RP_warning(): return 0 always
    - new functions:
        - RPF_IodeVersion(): new fn to retrieve the running version of IODE
        - RPF_SimMaxit, RPF_SimEps, RPF_SimRelax, RPF_SimSortNbPasses, RPF_SimSortAlgo, RPF_SimInitValues, RPF_SimNorm: new function to retrieve info about simulation parameters
        - RPF_SimNIter, RPF_SimCpu,  RPF_SimCpuSCC, RPF_SimCpuSort: data from last simulation
        - RPF_SimNormReal(), RPF_SimNIterInt() and RPF_SimCpuInt() to be shared bw report fns AND in python fns
    - added command $silent
    - added functions:
        - simsortnbpasses
        - simsortalgo
        - siminitvalues
        - simnorm
        - simniter
        - simcpu
        - simcpuscc
        - simcpusort
        - version
    - IodeInit(): does not read the SW_SEG_SIZE param in iode.ini anymore when the swap engine is already initialized (to enable the new -seg parameter of iodecmd).
    - IodeModelCalcSCC() and IodeModelSimulateSCC():
        new fns to implement in python the alternative simulation function model_calc_scc() and model_simulate_scc()
        based on pre-calculated lists
    - IodeModelSimNorm(char* period), int IodeModelSimNIter(char* period), int IodeModelSimCpu(char* period):
        new functions to retrieve simulations results / parameters
    - new vector KSIM_POSXK_REV[] containing the reverse of KSIM_POSXK[] to accelerate the process of retrieving
        the position of an equation in the model (for NEMESIS, reduces the SCC by a factor 300!)
    - KE_findpath(): assignation of KSIM_POSXK_REV[i] when KSIM_POSXK is modified

api/simulation/k_sim_main.c:
    - new vector KSIM_POSXK_REV containing the reverse position of the equations
    - new vector KSIM_CPUS containing the CPU time for each simulated period
    - K_simul():
        - initialize KSIM_POSXK_REV
        - alloc KSIM_NORMS, KSIM_NITERS and KSIM_CPUS
        - calc KSIM_POSXK_REV in the link loop
        - Fixed #330: in exchange endo-exo, if one of the vars does not exist, a message is
            provided and the function returns (and IODE does not crash anymore).
        - calc and store KSIM_CPUS[t] for each period

    - K_simul_free(): frees KSIM_POSXK_REV
    - K_simul_1():
        - initialize KSIM_CPUS[t]
        - added CPU in the iteration messages
    - K_simul(): fixed #1. In exchange endo-exo, if one of the vars does not exist, a message is
        provided (and IODE does not crash anymore).

api/k_sim_order.c:
    - new globals KSIM_CPU_SCC and KSIM_CPU_SORT
    - KE_order():
        - the message "sorting equations" is split in 2 steps:
        "Calculating SCC..." and "Reordering interdependent block...".
        The CPU is provided for each separated step and saved in KSIM_CPU_SORT and KSIM_CPU_SCC
    - KE_poseq():
        - use of KSIM_POSXK_REV containing the reverse position of the equations to (radically)
            accelerate the function
        - if posendo < 0 (ex in KE_ModelCalcSCC()), return(-1)
    - KE_tri(): save the elapsed time (not really CPU) in KSIM_CPU_SORT

api/k_sim_scc.c:
    - KE_ModelCalcSCC(): new variable KSIM_POSXK_REV to store reverse positions of endogenous variables
    - typedef LSTACK: misplaced closing comment */
api/iodebase.h:
    - declarations of new report functions:
        - int RP_silent(char* arg);
        - U_ch *RPF_IodeVersion(U_ch** args);
        - U_ch* RPF_SimSortNbPasses();
        - U_ch* RPF_SimSortAlgo();
        - U_ch* RPF_SimInitValues();
        - IODE_REAL RPF_SimNormReal(U_ch** args);
        - int RPF_SimNIterInt(U_ch** args);
        - int RPF_SimCpuInt(U_ch** args);
        - U_ch* RPF_SimCpu(U_ch** args);
        - U_ch* RPF_SimCpuSCC();
        - U_ch* RPF_SimCpuSort();
    - signature of T_view_tbl() reset
    - declaration of T_view_tbl_super reset
    - added in comments pro memory the temporarily changed signature of T_view_tbl to T_view_tbl(char* name, char *smpl, char** vars_names)
    - idem for T_view_tbl_super()
    - added declaration of W_printfReplEsc(char* fmt, ...)
    - signature of T_view_tbl() reset to its original value (+ in comment the signature used in IODE-QT)
    - definition of T_view_tbl_super reset to original values

api/report/undoc/b_view.c
    - B_ViewPrintVar(): reset to iode-dos version
    - B_ViewPrintTbl_1(): idem
	- SimulationTest : XNATY expected value 0.80071 replaced by 0.800703
@alixdamman
Copy link
Collaborator

From the logs of the last tests, one can see that the OS as been updated as well as the version MSVC -> 1935.
However, we still have the error for the test SimulationTest.SimulateSCC:

unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.

I probably made an error when compiling on my PC. I don't know.

The error is most probably as you said due to a constant string passed to a C function which modified it.

@alixdamman
Copy link
Collaborator

I will again make a checkout pf your branch jmp_sync_670 on my PC.
Recompile everthing and rerun all tests.

@alixdamman
Copy link
Collaborator

alixdamman commented Oct 16, 2023

And as in GH Actions, the test SimulationTest.SimulateSCC also fails on my computer

@alixdamman
Copy link
Collaborator

alixdamman commented Oct 16, 2023

@jmpplan
Note: the error SEH exception with code 0xc0000005 is thrown from the end of the C API function K_simul_SCC() (file k_sim_scc.c):

    for(i = 0; i < smpl->s_nb; i++, t++) {
        if(rc = K_simul_1(t)) goto fin;
    }

The K_simul_SCC() is called from the method Simulation::model_simulate_SCC() defined in the file iode\cpp_api\compute\simulation.cpp .

@alixdamman
Copy link
Collaborator

One step next:
In the C function K_simul_1() (file api\simulation\k_sim_main.c), the global variable KSIM_CPUS seems undefined when the function is called.
So, the line:

    KSIM_CPUS[t] = 0;  

returns the error SEH exception with code 0xc0000005

    - KSIM_CPUS allocated in K_simul_SCC_init()
    - KSIM_CPUS freed in K_simul_SCC_init()
@alixdamman
Copy link
Collaborator

alixdamman commented Oct 16, 2023

OK. Super. Tests are working!
Now, let me some time for the review...

@alixdamman alixdamman self-requested a review October 16, 2023 09:08
Copy link
Collaborator

@alixdamman alixdamman left a comment

Choose a reason for hiding this comment

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

As requested, I would like you to check the C++ Simulation class and add TODO comments (or recommendations in the code) when necessary.

    - Simulation::model_calculate_SCC(): reference to IodeModelCalcSCC() added
    - Simulation::model_simulate_SCC(): ref to IodeModelSimulateSCC() added + TODO

cpp_api/compute/simulation.h:
    - added some TODOs (implementation of a fns similar to IodeModelSimNorm, ...)
    - added two separated sections for VS 2019 and VS 2022 (to comment or uncomment):
        - cmake_minimum_required 3.20 vs 3.24
        - CMAKE_CXX_STANDARD vs add_compile_options("/std:c++latest")
@alixdamman alixdamman self-requested a review October 16, 2023 17:02
Copy link
Collaborator

@alixdamman alixdamman left a comment

Choose a reason for hiding this comment

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

OK for merge

@jmpplan jmpplan merged commit a067fe0 into master Oct 17, 2023
7 checks passed
@jmpplan jmpplan deleted the jmp_sync_670 branch October 17, 2023 10:40
alixdamman added a commit that referenced this pull request Oct 18, 2023
…de_model_simulate_exchange() function

-> returned value changed due to changes in the C API pushed in the PR #373
alixdamman added a commit that referenced this pull request Oct 18, 2023
…de_model_simulate_exchange() function

-> returned value changed due to changes in the C API pushed in the PR #373
alixdamman added a commit that referenced this pull request Oct 26, 2023
-> reset arguments to (TBL* tbl, char* smpl, char* name)

fix #387
alixdamman added a commit that referenced this pull request Oct 26, 2023
-> reset arguments to (TBL* tbl, char* smpl, char* name)

fix #387
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants