-
Notifications
You must be signed in to change notification settings - Fork 1
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
19 graphics #55
19 graphics #55
Conversation
-> Add dependency in |
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.
@DariaPigasova
These are my first comments. After resolution I will do a second review mainly on the algorithm itself and remaining changes.
Co-authored-by: Marcel Schmalzl <[email protected]>
Arguments should be:
|
Don't forget to resolve this. |
@DariaPigasova Do not mix double (") and (') single quotes.
|
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.
@DariaPigasova Do not mix double (") and (') single quotes.
- In Emma we always use double quotes
Not done
Emma/emma_libs/memoryManager.py
Outdated
# Plot objects | ||
drawElements(image, getElementsToPlot("Object_Summary"), startPoint, y2, svgwrite.rgb(198, 233, 175)) | ||
imageHeight = drawElements(image, getElementsToPlot("Object_Summary"), startPoint, y2, svgwrite.rgb(198, 233, 175), scaling) + 15 | ||
image.update({"height": str(imageHeight * float(scalingValue)), "width": imageWidth * float(scalingValue)}) |
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.
Possibly it is better to get the max dimensions of all objects and only then create the drawing. Like that we would spare us the additional update.
Emma/emma_libs/memoryManager.py
Outdated
try: | ||
float(value) | ||
except ValueError: |
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 is not the way to go to check if a correct value was entered.
- Other errors than
ValueError
can appear
2.In this case a try-catch might be O.K. but it has to be handled correctly.
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.
Also: not all previous reviews were done.
Emma/emma_libs/memoryManager.py
Outdated
@@ -184,6 +195,157 @@ def createStandardReports(): | |||
# graph.node('C', 'C', _attributes={'shape': 'triangle'}) | |||
# | |||
# print(graph.source) | |||
def createSvgReport(startPoint, endPoint, scalingValue="1"): |
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.
Please add x/y scaling: https://svgwrite.readthedocs.io/en/latest/classes/mixins.html#svgwrite.mixins.Transform.scale
The command line arguement would look like: --scaling=1:2
(x:y).
Also please merge the master into this file and resolve the conflicts. |
ToDo's / checks
warning
for--noResolvOverlap
and--memVisResolved
behaviour (as discussed)sc().warning("Incompatible arguments --noResolvOverlap
and--memVisResolved
were found. SVG figure will depict the unresolved scenario.")`svg
.(Unit) tests have been added / updated.svgwrite
in documentation3.2.3-> 4.0.0