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
Integrate autoreporting mvs #284
Conversation
@mahendrark - check the changes I made here to successfully run the code :) |
Doesn`t this solely implement parsing the argument that initiates the pdf report? I can not see that F2 is actually integrated ie. executed by the mvs jet. Can you change the title of this PR in that case? |
I wrote the function |
Ah, okay! I did not find your comment about the server anymore, so I was not sure if you wanted to tackle this here, too. |
d35ca58
to
1debd24
Compare
@mahendrark, please make your changes to the F2 reporting from this branch and as PR towards this branch |
@mahendrark - could you push your changes so we can merge them in this branch and merge this branch in I can make the code more general, however I understood you were tackling this issue so I don't want to step on your toes, let me know :) |
@Bachibouzouk |
Nice @Bachibouzouk , that you have figured out how to store the report as pdf! I am exited to see the the report being more integrated to the mvs @mahendrark :) Let me know if you do not know how to acess specific values in the PR request that you create for that. |
The solution to that issue is basically plotting the graphs in F2, instead of looking for the images of the plots in Outputs folder. Shall I plot all the necessary graphs with Plotly then? |
The quick and dirty solution is to add .png image one after the other at the end of the report, I would go for this one at first so that it unlocks #284. I think plots with plotly make sense then :) @smartie2076 has the last word on that though |
I agree, lets keep the png for now. For that, I could also adapt the resulting json so that it includes the paths to all generated plots. We can talk about that in our call later. |
Hi @Bachibouzouk ! There are actually issues before I or mahendra can continue working on this PR. First issue, when executing ´python mvs_tool.py -i inputs -f -ext csv`. It does not terminate the simulation or anything:
Second issue, when executing ´python src/F2_autoreport.py´ (even after installing ´wkhtmltopdf ´, which has to be added to the requirements.txt), the pdf is not generated:
I will probably do some coding on Friday, in case we need to have a call on this. |
I know the code does not work smoothly, that is what @mahendrark should solve :) (i did comment lines so that the code works on my end) |
Do you know if the solution to issue 1 and issue 2 is one and the same for both? Because the link to ´inputs/project_data.csv´ seems to be something with relative paths (issue 1), while for issue 2 basically might be wrong because of any of the png. Is there a way to print the name of the file that it can not find? |
Issue for 2) might be Windows ... I didn't have it... |
Issue 1) will be solved by @Bachibouzouk. Issue 2 is due to issues installing wkhtmltopdf, and the output was generated now. I will create a dictionary of paths to the created png files so that they can easily be called from F2. For this PR, we do not need the plotlydash plots but will only use pngs. @mahendrark : Please refer to this on how to get the pdf to compile! |
With the last commits, I created a dictionary of paths to plots, that can be accessed for the pdf report. This is now included in ´json_with_results.json´:
The simulation does not run through due to issue 1. Also, I have not added tests for the generation of this dict yet (not sure if I have to, as they will be removed with plotlydash plots anyway). I need issue 1 to be fixed first to fix the failing tests. I have indicated in following pdf (from standalone execution of F2) where the different plots should go. @Bachibouzouk - please look out for PF, those messages are for you, @mahendrark please look out for MR, those remarks are for you. @Bachibouzouk : Do you want to take over here and reference the paths in F2? This would take a long time for me to understand, and I would have no clue if simple loops would work. You can find the constants with which to access the json dict of plots here. @mahendrark please wait for @Bachibouzouk to be done with integrating the png, before you start finetuning the rest. |
@smartie2076, @mahendrark - it does run through now, however pdfkit has a problem to embed the images in the outputed pdf. In my last commit I indicated where you can increase the time to be able to vizualise the report in your browser before the "print_to_pdf" command is executed :) |
Thank you @Bachibouzouk ! The problem seems to have to do with the time allowed to store the pdf or something.
The images are not stored: out.pdf
Nothing is stored at all: out.pdf |
Okay, there still are 14 failing tests, but that doesn´t stop us from working further on this. @mahendrark we can have a call on what you can do today or tomorrow. |
This time is the time one waits before executing the command. I think the problem lies within the html to pdf converter, because if you set 60 seconds, then you can visit 127.0.0.1:8050 and you see the whole report. Another problem might be that the dash renderer is not properly called by the pdf converter... (The renderer turns tthe phyrhon code into html code which is then rendered by. Browser) |
I know that the Another question: Would it easily be possible to store the generated html? That could help us to finetune the formatting while the pdf is not generated. Also, later when we include dashplots, it may retain the interactive nature of the plots...? |
Sorry for the multiple pushes, somehow the pytests locally result in something different then the travis tests :/ |
tests/test_F0_output.py
Outdated
PLOTS_COSTS, | ||
) | ||
|
||
dict_plots = { |
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 would tend to create such a dict_plot
in constants.py
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.
Ah... yeah, makes sense. But don´t I need to use .deepcopy()
then to make sure that the changes in eg. dict_values
are not applied to the dict_plots
in the tests 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.
d1 = dict(a=1, b=2)
d2 = dict(c=3)
d2.update(d1)
print(d2)
d1['d']=4
print(d2)
d2 is not modified, so probably updates
takes care of that for you :)
Before it was fetching simulation settings from csv files, however, by only json should by used.
Adn import test constants from tests/constants.py instead of src.constants
d69aadc
to
34db9d8
Compare
Hey all, I am getting this error when I try to run f2 script: Traceback (most recent call last): |
Hi @mahendrark ! As this PR is closed, can you please create an issue and report a bug with the bug report template? This will also help us to keep track of existing bugs and whether they are solved. Also, if you include me/PF with @+github handle, we automatically receive a mail :) |
@smartie2076 thank you Will do that right away :) |
Fix #282
Fix #310
Changes proposed in this pull request:
Fixed:
The following steps were realized, as well (if applies):
black . --exclude docs/
)pytest tests/test_benchmark.py
)For more information on how to contribute check the CONTRIBUTING.md.