-
Notifications
You must be signed in to change notification settings - Fork 134
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
FEAT: ISAR plot #5639
FEAT: ISAR plot #5639
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5639 +/- ##
==========================================
+ Coverage 81.38% 85.24% +3.86%
==========================================
Files 152 152
Lines 60952 61008 +56
==========================================
+ Hits 49606 52007 +2401
+ Misses 11346 9001 -2345 |
src/ansys/aedt/core/visualization/advanced/rcs_visualization.py
Outdated
Show resolved
Hide resolved
src/ansys/aedt/core/visualization/advanced/rcs_visualization.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Maxime Rey <87315832+MaxJPRey@users.noreply.github.com>
Co-authored-by: Maxime Rey <87315832+MaxJPRey@users.noreply.github.com>
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.
Overall this changes look good to me, thanks for the feature @Samuelopez-ansys
None the less, I would like to discuss two points:
- Should we use
casefold()
here instead oflower()
? I discovered this method in FIX: Fixes to get_object_material_properties method #5626 and it seems to be designed specifically for case-insensitive comparisons. This would allow users use special cases in certain languages or alphabets. However, we might not want to allow that :D - Instead of adding a new file to the pyaedt repo, and since the JSON data is tiny, could we avoid adding the JSON file and create it on the fly while testing ? For example, at the begining of the file we could have a constant with the JSON file content and in the test's setup, we could create the file with
json.dumps(...)
?
I marked this review as request changes but it's more to ensure that a discussion occurs than a real need for changes :)
src/ansys/aedt/core/visualization/advanced/rcs_visualization.py
Outdated
Show resolved
Hide resolved
src/ansys/aedt/core/visualization/advanced/rcs_visualization.py
Outdated
Show resolved
Hide resolved
src/ansys/aedt/core/visualization/advanced/rcs_visualization.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Sébastien Morais <146729917+SMoraisAnsys@users.noreply.github.com>
Co-authored-by: Sébastien Morais <146729917+SMoraisAnsys@users.noreply.github.com>
Co-authored-by: Sébastien Morais <146729917+SMoraisAnsys@users.noreply.github.com>
@SMoraisAnsys I applied the casefold, I like it, and also I removed the JSON files in the test, and it creates them on the fly |
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.
LGTM, nice to discover the use of the object request
in pytest !
Description
Improve ISAR RCS plot
Issue linked
Close #5641
Checklist