-
Notifications
You must be signed in to change notification settings - Fork 7
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
Floating point support #58
base: master
Are you sure you want to change the base?
Conversation
rawspec_testing bash command success is contingent on both of these conditions:
Rule of thumb: Don't write to stderrr unless exit status is nonzero. As an aside, Python and other languages logging provide other useful logging types (warning, debug) with good reason. Motivation for checking stderr contents in regression testing: I previously found some places in the rawspec source files where there were errors but a zero exit status was being passed back. It is important to catch these cases. Fixes applied in order to complete regression testing rawspec_gpu.cu
Since this was an informational-only message, it should go to stdout. Warnings and errors are considered failures in regression testing.
Changed them both to stdout. rawspectest Produces erroneous output in rawspec_testing which checks the output to baseline results. ==================================================================== Finally, all rawspec_testing succeeds given changes made that I indicated earlier. Note that these are 100% integer cases. We could expand rawspec_testing with the addition of a floating-point .raw file from ATA. cc: @david-macmahon |
Thanks Richard! I'll actually switch the |
Please make the stderr --> stdout changes too. Thanks. |
Okay, success on all SIGPROC and FBH5 cases. |
@radonnachie Fixing issue #61 (closed) caused this PR to fall slightly behind rawspec main. I suggest that you pull the changes. Is this still a work in progress? |
d714cf5
to
d702da5
Compare
Thanks texadactyl. It's in production use at the ATA, no issues. Only thing left to do is add test cases for floating-point input data. I believe it last passed all the integer tests. |
@wfarah @radonnachie |
@wfarah @radonnachie |
It's fairly common place now correct. I believe it is primarily used for calibration. I can. Thanks texadactyl |
Using the provided rawspec parameters ..... rawspec ./ATA_guppi_59730_78085_225768981_J0332+5434_off_0001-beam0000 -f 1 -t 32 -d .
Header:
|
Sure, the provided file needs to be processed by rawspec as it is in this branch. Then nbeams kicks in and there will be a printout as per Line 584 in d702da5
|
@radonnachie I installed the PR code from the right branch. Also, I thought the right field name is NBEAMS? |
ed6f7c3
to
9dc34bf
Compare
?????
|
It is NBEAMS, I've updated that printout. Thanks. I appreciate that you would know to have installed the PR code, but the NBEAMS override of nants happens before that check: I have:
|
9dc34bf
to
139c340
Compare
I installed from here: After my last git pull + make:
Better, but the version is off. |
Last git pull:
|
Ok, last git pull still has wrong version BUT it ran to conclusion with the supplied .raw file.
|
The addition of I know the original meanings of the GUPPI RAW headers are not well documented, but there is historical precedent for the header names that predate TBH, storing multiple beams in a single GUPPI RAW data block is not really that great of an idea since the headers only have a single values for RA, DEC, SRC_NAME, etc. I think keeping a single beam per GUPPI RAW file is cleanest in terms of backwards compatibility, but if one really wanted to add support for storing data from multiple beams in a GUPPI RAW file it would probably make more sense from a file format and metadata perspective to store a single beam per GUPPI RAW data block and allow the |
Great points. Thanks for the context which supports understanding, @david-macmahon . Keenly, you've highlighted a lapse in my explanation for how we're employing it. Currently, NBEAMS only ever gets a value of 1. I think your statement is a far better employment of the NBEAMS key. I'll adjust the following, and hopefully this chimera is simple enough that I don't have to split it up:
|
b4d31f8
to
b541eae
Compare
Finally I guess that I could fully kill the NBEAM part of it, leaving only the floating point support, by just making my implementation set NANTS=1. |
Et voila. Again, thanks for your patience. |
@radonnachie Have you run the regression testing (rawspec_testing)? Just to make sure that no inadvertent side-effects occurred. |
- should handle floating point input (half/single)
- better align with integer data being the default
5e23e8c
to
bd8dbab
Compare
@radonnachie
The other files (GBT and ATA) had no issues. Baseline: files/results from current rawspec on BL github. File header:
|
I'm certain that this is because the baseline is interpreting the sample-data of the That all of the other files pass indicates that nothing has changed regarding the integer sample-data processing, so I believe you have provided justification for this to undergo final review. I think @david-macmahon mentioned something about stipulating which kinds of floating point formats are supported. Asides from the categorical 16 and 32 bit limitations in the code, I'm not sure I see what else could be stated. Maybe that it only handles the in-built float type, IEEE754. Thanks for testing @texadactyl, I feel pretty guilty for still not knowing how to 😶. |
@radonnachie I take it that the "trial" floating-point values reported in this issue stream by me are correct for the intended file contents. These will be used to update the rawspec_testing baseline directory once @david-macmahon has approved the merge. |
This statement confused me: "In the presence of NBEAM > 0, take there to be only 1 BEAM present and use 1 instead of NANTS. This ultimately means split-ant and ics modes are no-ops in the presence of NBEAM>0". Are you saying that headers should have NBEAM or NANTS present but not both? If so, the presence of both should cause a stderr diagnostic message that clearly states which is in force. Already does? If NBEAM is active, then this is the same as a non-antenna case (E.g. GBT). True? If NBEAM > 0, rawspec ignores all but the first block? If so, a warning should be issued when blocks are ignored or the operations documentation should indicate this. This was also puzzling: "It would be good to specify which 16-bit floating point format(s) is(are) supported.". What formats could there be? Could there be more than one? Sorry if my inexperience is showing. rawspec needs an operations manual and an update to https://github.com/UCBerkeleySETI/breakthrough/blob/master/doc/RAW-File-Format.md |
I think I had run my mouth a bit. Ive added no capability that uses the NBEAM value. So that hasn't changed. I agree with the state that rawspec was in before this PR: NBEAM isn't used other than to populate the filter bank header. So it's expected that beam populated files have NANTS set to 1 if at all. The mouth running I did should fall into another PR, if it's worthwhile (I don't think it is), like Dave mentioned. All that's outstanding is the floating point format curiosity... |
@radonnachie If only we were in the same office area with access to a shared whiteboard and able to ask questions in real-time. Half (or more) of these electronic messages would be unnecessary. Don't beat yourself up! (-: |
Change log entry would be something like:
|
Triggered (for now) by DATATYPE == 'FLOAT', which is not the fallback value ('INTEGER' is).
#41