-
Notifications
You must be signed in to change notification settings - Fork 125
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
Basic script to compare two BP files given a tolerance #4308
Conversation
804d566
to
94f20e2
Compare
@vicentebolea could you help me with these errors? On macos-14 where numpy is found:
And on Ubuntu 20-04
|
@anagainaru I recommend you to use this add_test wrapper: ADIOS2/cmake/ADIOSFunctions.cmake Line 72 in 4026559
The reason is that it does all the fixes so it uses the correct python version and pythonmodule var |
Thanks ! Can I use |
@vicentebolea I don't seem to know how to use the python test you indicated. In my case, my testing is not a python file, it's a pipeline (similar to bpls but it has a python script in the middle). Any advice helps, it's not straight forward to know how to take some parts of the |
I See, you need to extract from the python_add_test the parts that setup the needed env variables for finding adios2 python package in the build tree and also the correct python interpreter (as you already done).
|
Setting the env for python did not work, @vicentebolea any other ideas? |
You can set env variables for a command
…On Wed, Aug 21, 2024 at 5:12 PM Ana Gainaru ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In testing/utils/bpcmp/CMakeLists.txt
<#4308 (comment)>:
> + ENVIRONMENT
+ "PYTHONPATH=${ADIOS2_BINARY_DIR}/${CMAKE_INSTALL_PYTHONDIR};$<SHELL_PATH:$<TARGET_FILE_DIR:adios2_py>>;$ENV{PYTHONPATH}"
+ "PATH=$<SHELL_PATH:$<TARGET_FILE_DIR:adios2_py>>;$<SHELL_PATH:$<TARGET_FILE_DIR:adios2_core>>;$ENV{PATH}"
+ "PYTHONUSERBASE=${CMAKE_BINARY_DIR}"
Actually that is not going to work, because in the bpcmp.cmake.gen I am
executing a command, not setting a test, and you do set_property on a
test.
—
Reply to this email directly, view it on GitHub
<#4308 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHFOFRJLEVG3NRTFGFKQNTZST7C5AVCNFSM6AAAAABMSIEF5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENJRHE2TGMBQG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
[image: logo shows large letter K with the word Kitware below it]
<http://www.kitware.com>
Vicente Bolea, MSc.
*Senior R&D Engineer*
|
Using set() would be enough? Something like:
I've never set these before. |
Yes, it should work
…On Wed, Aug 21, 2024 at 5:29 PM Ana Gainaru ***@***.***> wrote:
Using set() would be enough?
Something like:
set(ENV{PYTHONPATH} "All/the/paths/in/your/wrapper")
I've never set these before.
—
Reply to this email directly, view it on GitHub
<#4308 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHFOFWSARG3X535QQOJKILZSUBEDAVCNFSM6AAAAABMSIEF5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBTGA2DINZWGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
[image: logo shows large letter K with the word Kitware below it]
<http://www.kitware.com>
Vicente Bolea, MSc.
*Senior R&D Engineer*
|
a3859c8
to
decbe80
Compare
Co-authored-by: Norbert Podhorszki <pnorbert@ornl.gov>
Co-authored-by: Vicente Adolfo Bolea Sanchez <vicente.bolea@kitware.com>
@vicentebolea the tests pass ! Thanks for the help. Could you please see if this looks good to you? |
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.
Looks good, a few suggestions. I will review the python script tomorrow. Good work @anagainaru 🚀
@vicentebolea thanks for the suggestions. I pushed some of the changes you mentioned. The other changes that I did not push do make sense, but I would prefer leaving this PR consistent with the other utility scripts (the functionality and build system is mimicking bpls -- namely using PROJECT_BINARY_DIR, not using quotes for paths, not using syntax=yaml, not using the maybe convention). I think all these changes need to be done to all the scripts in |
You are right that this would break consistency which is very important, if you have extra cycles it would be cool to have the other utils things "Corrected" otherwise I am to strong about it. As for the |
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.
Few code change request in the Python code. Looks goods, feel free to take the suggestions
On the funny side of things I was reading this article https://www.chiark.greenend.org.uk/~sgtatham/quasiblog/code-review-antipatterns/ . I believe that I might have been guilty of some of those antipatterns haha. Ill take those lessons learned in my next reviews 👼 |
eb71e41
to
8267f27
Compare
Co-authored-by: Vicente Adolfo Bolea Sanchez <vicente.bolea@kitware.com>
Based on a request from GX developers, this is the first version of a python script to compare the information in two BP files (given a certain tolerance). The functionality is kept similar to https://gitlab.com/remikz/nccmp
Current differences detected:
--stats
option is usedThe tolerance can be absolute value or percentage.
Example metadata differences:
Example min/max differences:
Without verbose the script only plots a summary of the value difference
Using
--verbose
the script will plot every value differenceThings that still need to be added: