Skip to content
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

Unable to Load FITS files with NaN values #75

Closed
Mulan-94 opened this issue Oct 6, 2020 · 12 comments
Closed

Unable to Load FITS files with NaN values #75

Mulan-94 opened this issue Oct 6, 2020 · 12 comments

Comments

@Mulan-94
Copy link

Mulan-94 commented Oct 6, 2020

Hi Eric,

I am unable to load an image masked with NaN values to JS9

Version: 3.1
The browser tab crashes with the follwing error: Error Code: SIGSEGV.

This happens for both my local, and the online JS9. However, replacing the NaNs with zeroes allows the image to be loaded. Would you know what is going on?
Thanks.

@ericmandel
Copy link
Owner

@Mulan-94 Hi Lexy, I'll need some more info, because below is a test image containing NaN values that loads correctly. If you have an image containing NaN values that crashes, could I please get a copy? Or, if you are using JS9.MaskImage(), can I get a copy of both the mask and the image being masked?

Screen Shot 2020-10-06 at 7 45 40 AM

@ericmandel
Copy link
Owner

@Mulan-94 Also, once we have a file that reproduces the problem, we'll need to know how the NaN values are made. I see that the Posix nan() call takes an argument and I just passed a NULL. Of course, there are other ways to make a NaN. Without any data whatsoever, I see two possibilities:

  1. if the segv is happening in the CFITIO library, we can run the ftools fverify on the file, test against a CFITSIO desktop program, perhaps submit a bug report to HEASARC

  2. if the segv is happening in Javascript, we'll want to make sure that the standard Number.isNaN() is handling your NaN values correctly, we might require a work-around.

@Mulan-94
Copy link
Author

Mulan-94 commented Oct 6, 2020

@ericmandel sorry just seeing this, I'm emailing you the image now

@ericmandel
Copy link
Owner

@Mulan-94

When I open CYG-FPOL-FLO.FITS using JS9 (Desktop version), I get the following error:

Screen Shot 2020-10-06 at 12 11 39 PM

And looking at the file, I see the header is really long: 2822400 bytes or 35280 80-byte header entries, almost all of it HISTORY keywords. Following wcslib defaults (and after long conversation with Jessica Mink, wcslib author), I set a max header size to 256000 bytes, or 3200 header entries.

I'm not sure what to do about this. Off-hand I forget what the trade-offs are for increasing the wcsinit max size and I will look into this. In the meantime, can you tell me if this is an anomaly or do you expect files header of this size typically?

cc: @o-smirnov

@Mulan-94
Copy link
Author

Mulan-94 commented Oct 6, 2020

I'm not sure actually, let's wait for @o-smirnov on this one

@ericmandel
Copy link
Owner

@Mulan-94 @o-smirnov I've sent email to Jessica Mink (instead of walking over to her office, sigh), asking for advice on increasing the max header parameter. The relevant code looks benign, so I hope we will be able to do this. I'd probably try 5Mb or 10Mb to start ...

@ericmandel
Copy link
Owner

@Mulan-94 @o-smirnov

OK, there are two problems here: at first I thought that wcsinit() had to be passed a larger value for hlength (max length to check for a header param in case END is not found, default 256000). But now I believe that the current hlength is fine in your case: all of the relevant header parameters are way before the 256000 byte limit. That is, even with this too-small limit, all params should be found correctly.

More problematic is the Emsripten stack limit problem:

emscripten-core/emscripten#11135

By default, Emscripten has a 5Mb stack limit. Strings are passed on the stack using 4 * string size + 1 bytes, so your 2.8Mb header requires > 10Mb of stack space and ... SEGV time.

I don't want to allocate a semi-infinite amount of stack space for the occasional huge size header FITS file, so I'm gonna have to figure out how to call wcsinit() more cleverly. Stay tuned ...

@o-smirnov
Copy link
Contributor

Thanks for looking into this @ericmandel! That header is pretty excessive but, sadly, not exceptional for radio images produced by AIPS. It likes to be very verbose about its data processing history. So while we can easily work around it in each individual case, this is something that will continue to trip the unwary user...

@ericmandel
Copy link
Owner

@o-smirnov @Mulan-94 That's fine, I have practically no experience with radio data and was hoping it was a mistake!

I just have to copy the header string to the Emscripten heap (and pass a pointer) instead of passing the string directly on the stack ... once I get that technique working, it should work forever ...

@ericmandel
Copy link
Owner

Still a way to go, but the heap-passing technique is starting to look promising ...

Screen Shot 2020-10-06 at 3 51 03 PM

@ericmandel
Copy link
Owner

@Mulan-94 @o-smirnov OK, we're all set and updated in GitHub. Thanks, Lexy, for pointing this one out, it was a very interesting bug!

@Mulan-94
Copy link
Author

Mulan-94 commented Oct 7, 2020

Great! and thanks @ericmandel for taking your time to look into it so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants