-
Notifications
You must be signed in to change notification settings - Fork 45
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
f/shashank-website -- Website Documentation -- Creation of Website using Python #46
f/shashank-website -- Website Documentation -- Creation of Website using Python #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we need to start with a reorganization. Move this project to project/website. Consolidate all source to simply project/website/src and simplify your CMakeLists.txt to a minimal clean cmake file. There are lots of extra directives in your two files that are just carrying over legacy structure that is not needed for this new addition.
Once your done with that I'll fully review the python source.
projects/CMakeLists.txt
Outdated
@@ -152,7 +152,7 @@ add_subdirectory(unit) | |||
add_subdirectory(cmd_bio) | |||
add_subdirectory(circuit_profiler) | |||
add_subdirectory(zip) | |||
|
|||
add_subdirectory(python) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also want you to remove all references to JAVA in the build system. Delete the project/java and any cmake commands related to it.
set(PROJECT_NAME Python_BioGears) | ||
set(PREFIX biogears_py) | ||
|
||
set(${PREFIX}_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/include" PARENT_SCOPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You likely do not need to keep this block of variables. They are left overs from an older way I propagated -I flags in C++ from file to file. As this is a python based project I doubt they are used anywhere outside of this file.
############################################################################### | ||
# Requirments | ||
############################################################################### | ||
if(Biogears_Python_BUILD_WEBSITE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add https://cmake.org/cmake/help/v3.14/module/FindLATEX.html to this. Its required for the full generation of the website to generate the BibTex entries might as well help CMake point this out.
|
||
add_source_files(SRCS ${CMAKE_CURRENT_SOURCE_DIR}/src "*.py" "Sources\\") | ||
|
||
set(${PREFIX}_HEADERS ${HDRS} ${PUBLIC_HDRS} ${GEN_HDRS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can afford to simplify this file a little. I know it breaks convention, but given we only have SRCS we can just refer to that variable in python projects.
|
||
string(REGEX REPLACE "Python_" "" SHORT_NAME ${PROJECT_NAME}) | ||
|
||
add_custom_command( OUTPUT ${SHORT_NAME} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has no command so its should be removed
############################################################################### | ||
message(STATUS "Configuring ${PROJECT_NAME}") | ||
|
||
SET(MD_MAIN_PAGE "MainPage.md") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you use this variable ?
projects/python/cdm/CMakeLists.txt
Outdated
set(PROJECT_NAME Python_CommonDataModel) | ||
set(PREFIX cdm_py) | ||
|
||
set(${PREFIX}_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/include" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these variables are not referenced go ahead and remove them.
projects/python/cdm/CMakeLists.txt
Outdated
############################################################################### | ||
|
||
add_source_files(SRCS ${CMAKE_CURRENT_SOURCE_DIR}/src "*.py" "Sources\\") | ||
add_source_files(SRCS ${CMAKE_CURRENT_BINARY_DIR}/src "*.py" "Sources\\") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have ant files in this folder? If not remove this call. I don't see anything generating files up to this point
projects/python/cdm/CMakeLists.txt
Outdated
set(XSDToDOXYGEN_SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/src/mil/tatrc/physiology/datamodel/doxygen/XSDToDoxygen.py PARENT_SCOPE) | ||
set(Plot_SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/src/mil/tatrc/physiology/datamodel/plots/PlotDriver.py PARENT_SCOPE) | ||
|
||
add_custom_command(OUTPUT ${SHORT_NAME} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command does nothing. Remove it
projects/python/cdm/CMakeLists.txt
Outdated
add_custom_command(OUTPUT ${SHORT_NAME} | ||
DEPENDS ${${PREFIX}_SOURCES}) | ||
|
||
add_custom_target(${PROJECT_NAME} ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe you use this anywhere. So this target is not needed.
It appears that at least two of your python files are completely unused. Remove any dead files from your python source. Description.py and CDMSerializer.py or explain there purpose. If they are functionality for a future pull request leave them out of this pull request and include them when they do have use. |
@StevenAWhite I have created the pull request with all of the suggested changes and adding the virtual environment based target as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have the plotter working you need to remove the comments in the command. If the comments are no longer needed you should remove them.
projects/website/src/CMakeLists.txt
Outdated
if(NOT VIRTUALENV) | ||
message(FATAL_ERROR "Could not find `virtualenv` in PATH") | ||
endif() | ||
set(VIRTUALENV ${VIRTUALENV} -p ${Python3_EXECUTABLE}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't override this here. Just use the paramaters in the command
Ok the next challenge is to let the plotter fall back when looking to results to baselines. A baseline is a zip file which contains Results files from a previous run of BioGears. CMake determines if a baseline exist by
Other then the Biogears_Baseline_DIR you can also look in a subdirectory baselines/ in the same folder that you expect to find a given *Results.csv file. I'm attaching a baseline for review its very simple. All a Baseline Dir is an exsact directory structure match to runtime/Scenarios where each folder has a baselines/ director and those baseline directories contain zip files of the Results.csv and Results.log file.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what I'm seeing. I want to see a thread pool added to the plotter to speed up its execution time. We can decide a default number of threads for CMake to pass latter, but for now I just want proof the threadpool works and we can plot say 3 plots at a time.
Really minor syntax clean up on your write calls just to clean up how that looks to other maintainers. The only other thing which isn't a huge deal is to teach the PlotDriver to fallback to baselines if it doesn't find a fresh results file. You likely want to log when you do this, but 90% of the time these tools will be used in an automated system that doesn't have 48 hours to run verification so we want to run against historical data if its there.
8185352
to
0f4a116
Compare
…ings will be stored
…ings will be stored
…ctions and if-else cases
… basic structure of PlotDriver to execute the PlotDriver.py file using Python and added PlotDriver class to it
…yle to make zip fle
…fig files and created a class for defining variables which i need to extract from the config file
…h is used to create a single object which contains all information related to config file which can be used for plotting
… initialize where actual file needs to be saved and to get the directory of baseline directory
…ions text and time from the zip file
…on to ActionEventPlotter script and added logging to the function based on occurence of Actions or Events
…s removing the python pyxb schema directory
…l environment target and changing the structure from python to website src directory and cleaning the old legacy code from CMake
…otDriver.py file added -j as an argument for specifying number of threads, XSDToDoxygen fout write cleaning and virtual environment overriding removal
…ting the baseline directory structure as suggested and done some other changes as well to plotting function
…for better plotting
…ated to virtual environment as well as fixing the code
…per said and handles signal quit which we can quit any thread
…l+c to pause and second to exit
…except blocks for better usability of the Plots
…tting of all graphs
…hich will help in creation of multiplotter graphs for different axis
8983a7f
to
82ccdff
Compare
…execution of targets
…_stabilization function and variable for keep track to generate patient table or not
…o generate table for patient as well some parsing is required also
Accepted |
Pull Request for Python Website solving issues #42 #44 #45 which is used for building website documentation using Python and removing Java dependencies.