-
Notifications
You must be signed in to change notification settings - Fork 13
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
Mods to support spectra file IO out of fortran #34
base: master
Are you sure you want to change the base?
Conversation
This looks like a nice addition - thanks! |
* added separate swig interface file * build tweaks for successful linking to shared libraries
* Then you can run ctest in the same dir you build in * Also update version in unit_tests
The beauty of doctest is you just have to include one header file.
unit_tests/test_spec_file.cpp
Outdated
//auto timesEqual = expectedM.start_time() == actualM.start_time(); | ||
auto timeStr1 = SpecUtils::to_iso_string(expectedM.start_time() ); | ||
auto timeStr2 = SpecUtils::to_iso_string(actualM.start_time() ); | ||
CHECK( timeStr1 == timeStr2); |
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.
@wcjohns I think we lose some time precision when we write and read pcf files. My test is failing when comparing measurement start times.
1: /mnt/c/Projects/code/SpecUtils/unit_tests/test_spec_file.cpp:81: ERROR: CHECK( timeStr1 == timeStr2 ) is NOT correct!
1: values: CHECK( 20240812T164010.285502 == 20240812T164010.29 )
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.
That's correct - PCF files store time in VAX format (a 23 character string formatted like "19-Sep-2014 14:12:01.62"), so only to the 100th of a second.
SpecUtils normally tracks down to the microsecond (via using time_point_t = std::chrono::time_point<std::chrono::system_clock,std::chrono::microseconds>
).
It looks like SpecUtils::to_vax_string(..)
rounds to the nearest 0.01 seconds, so you could test:
auto timeStr1 = SpecUtils::to_vax_string( expectedM.start_time() );
auto timeStr2 = SpecUtils::to_vax_string( actualM.start_time() );
CHECK( timeStr1 == timeStr2);
Or something like:
CHECK( std::round(0.0001*expectedM.start_time().count()) == std::round(0.0001*actualM.start_time().count()) );
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, thanks! I'll use to_vax_string
to compare.
@@ -11,6 +11,9 @@ endif() | |||
project( SpecUtils VERSION 2025.1.0 ) | |||
enable_testing() # Enable here to allow running `ctest` from top-most build dir | |||
|
|||
# Add this line to specify the architectures for MacOS | |||
set(CMAKE_OSX_ARCHITECTURES "x86_64;arm64" CACHE STRING "Build architectures for MacOS" FORCE) |
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.
@wcjohns so everything builds ok but macos*. Copilot said to add this but that didn't help.
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.
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 probably remove that added line; I think the macOS architectures are set by the nanobind stuff.
I just triggered a build on the main branch, and it ran fine.
It looks like the problem is that on this branch, it is installing all the headers, and I think crucially the static SpecUtils.a library, into the Python wheel. When delocate
is ran on the wheel, the .a file is causing the failure.
Below is the step that is installing stuff into the python package:
And the actual error:
Which looks to be being triggered by the .a file.
You're saying that installing the headers is causing it to break?
____________________________________________________
Hugh P. Bivens
Sandia National Laboratories
Albuquerque, NM USA
+1 (505) 284-6822
https://snl-wiki.sandia.gov/x/6ID2BQ
From: William Johnson ***@***.***>
Sent: Wednesday, January 29, 2025 5:27 PM
To: sandialabs/SpecUtils ***@***.***>
Cc: Bivens, Hugh P. ***@***.***>; Author ***@***.***>
Subject: [EXTERNAL] Re: [sandialabs/SpecUtils] Mods to support spectra file IO out of fortran (PR #34)
@wcjohns commented on this pull request.
________________________________
In CMakeLists.txt<#34 (comment)>:
@@ -11,6 +11,9 @@ endif()
project( SpecUtils VERSION 2025.1.0 )
enable_testing() # Enable here to allow running `ctest` from top-most build dir
+# Add this line to specify the architectures for MacOS
+set(CMAKE_OSX_ARCHITECTURES "x86_64;arm64" CACHE STRING "Build architectures for MacOS" FORCE)
We can probably remove that added line; I think the macOS architectures are set by the nanobind stuff.
I just triggered a build on the main branch, and it ran fine.
It looks like the problem is that on this branch, it is installing all the headers, and I think crucially the static SpecUtils.a library, into the Python wheel. When delocate is ran on the wheel, the .a file is causing the failure.
Below is the step that is installing stuff into the python package:
image.png (view on web)<https://github.com/user-attachments/assets/a55e5484-c0b4-4cb1-a78e-57823de454ef>
image.png (view on web)<https://github.com/user-attachments/assets/688c9184-2f77-4745-8b9b-5544b94df5a1>
And the actual error:
image.png (view on web)<https://github.com/user-attachments/assets/6ecc5b23-e2a3-46d8-9afc-cd8977ecb4a2>
Which looks to be being triggered by the .a file.
-
Reply to this email directly, view it on GitHub<#34 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADAP4BEL3FBAP6FG6EFTHJT2NFWWRAVCNFSM6AAAAABLPPWDBOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOBSGY3DQNBXHA>.
You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>>
|
This is in respect to trying to fix macOS CI build: #34 (comment)
It looks like its libSpecUtils.a that is causing the issue; I'm not sure why delocate is trying to do its thing on a static library, I thought it essentially fixed up shared libraries. I just pushed 4e70828 and 4f0f402, which changes the main CMakeLists.txt file so none of the install stuff is called, if the build is not a top-level project. E.g., if a python build. |
Yeah I don't think we need to install stuff on a python build. |
@wcjohns How do I set or get the energy cal low energy field? |
Its the 5th entry in the energy calibration coefficient parameters. |
Thanks! |
No description provided.