-
Notifications
You must be signed in to change notification settings - Fork 29
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
read sardana mca #129
read sardana mca #129
Conversation
thanks for the effort. The tests fail because of the data format change you did. I think this patch should fix this. (This is actually not your mistake but fixes the previous inaccuracy of float32!) I am not sure if I like the unconditional change of the MCA data format to float64. It seems waste of memory and traditional spec seemed to always use unsigned int values. Is there no way to detect what data format is needed? One could use the presence of Also I would like to include a basic unittest with the data file you submitted in the related issue. Are you fine to put this datafile to the other test data? does it require any special copyright notice? I would then upload it there and could contribute a very stupid test (similar to the other |
Hi, thanks for reviewing the PR. I will add the patch for the test. Regarding the memory waste for always using I would add another flag, such as |
this sounds good. I think we can assume only original SPEC and Sardana and
fix other problems when the come up (I somehow hope that other
implementations would copy the approach of either solution). Once you
include this flag and the current tests succeed I think I can handle the
rest in another PR.
…On Mon, Nov 8, 2021 at 9:57 AM Daniel Schick ***@***.***> wrote:
Hi,
thanks for reviewing the PR. I will add the patch for the test.
You can add the datafile to your test data without any notice. It would be
great if you could add the required test.
Regarding the memory waste for always using float64 for the MCA:
If we assume that there is just original SPEC and Sardana which create
.spec files, checking for Det_x sould be fine.
I would add another flag, such as has_sardana_mca which is set when
parsing the file and searches for Det_x.
Then within parsing the scan, I could initialize the MCAs either with
float64 or as uint
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#129 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACKZJFJBOJ7IA6QTMCTCCBDUK6GIRANCNFSM5HLZOOZQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
I just noticed, that I have not taken care about the hdf part of the SPEC IO |
Hi @dschick , |
hi, thanks for the reminder. I will try to finish this in the next week. |
@dschick Can I merge this PR as is to fix the MCA issue? Of course it would be nice to have the HDF5 part also fixed, but likely this is secondary over being able to read the file in a first place. |
Hi @dkriegner sorry for not finishing this PR. But I agree that the spec-file part should be merged as is. Thanks Daniel |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
the failing tests seem to have nothing to do with this PR (they occur since "today" also on other pull requests, but not locally). I need to nevertheless investigate this before merging. |
this PR allows to read MCAs from origininal SPEC files as well as from Sardana generated SPEC files which can have multiple MCAs.
In addition the data type for the MCAs and scalar data has been changed from
int32/float32
tofloat64
.