Skip to content

FIREFLY-116:Evaluate the wavelength readouts from various types of FITS and fix any Errors #827

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 20, 2019
Merged

FIREFLY-116:Evaluate the wavelength readouts from various types of FITS and fix any Errors #827

merged 1 commit into from
Jun 20, 2019

Conversation

lznakano
Copy link
Contributor

The wavelength implementation is carefully evaluated based on algorithm described in three reference papers. Default values are enforced. Three bugs are fixed. Please see ticket https://jira.ipac.caltech.edu/browse/FIREFLY-116 for more information.

Test URL: https://irsawebdev9.ipac.caltech.edu/FIREFLY-116/firefly/

@lznakano lznakano requested review from robyww and ejoliet June 15, 2019 00:22
ejoliet
ejoliet requested changes Jun 17, 2019
View reviewed changes
Copy link
Contributor

@ejoliet ejoliet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, almost there!

I've tested most of the files attached and i can see now the wavelength in the toolbar when is a cube.

Unfortunately, the Spherex sample are not showing the wavelength in the readout. Unless i'm wrong, i thought the Spherex files are following pretty much the standard of spectral coordinates (spectral image).
Could you look at those sample and see why is not showing up in the readout? Thanks!

@lznakano
Copy link
Contributor Author

@ejoliet
Please take a look at my comment in FIREFLY-116.

@ejoliet
Copy link
Contributor

ejoliet commented Jun 17, 2019

Ok, i'll take a look but just for the records here, i did tested 'test' and 'testLinear' files and wavelength is shown inn the display! Yay!!
The one that is not has this header:

# HDU 0 in LVF-position-20-Ch-1.fits:
SIMPLE  =                    T / conforms to FITS standard                      
BITPIX  =                  -64 / array data type                                
NAXIS   =                    2 / number of array dimensions                     
NAXIS1  =                 2048                                                  
NAXIS2  =                 2048                                                  
WCSAXES =                    3 / Number of coordinate axes                      
CRPIX1  =               1024.0 / Pixel coordinate of reference point            
CRPIX2  =               1024.0 / Pixel coordinate of reference point            
CRPIX3  =                  0.0 / Pixel coordinate of reference point            
CDELT1  =  -0.0017222222222222 / [deg] Coordinate increment at reference point  
CDELT2  =   0.0017222222222222 / [deg] Coordinate increment at reference point  
CDELT3  =  2.7997850569788E-10 / [m] Coordinate increment at reference point    
CUNIT1  = 'deg'                / Units of coordinate increment and value        
CUNIT2  = 'deg'                / Units of coordinate increment and value        
CUNIT3  = 'm'                  / Units of coordinate increment and value        
CTYPE1  = 'RA---TAN'           / Right ascension, gnomonic projection           
CTYPE2  = 'DEC--TAN'           / Declination, gnomonic projection               
CTYPE3  = 'WAVE'               / Vacuum wavelength (linear)                     
CRVAL1  =       151.8596107642 / [deg] Coordinate value at reference point      
CRVAL2  =      1.8814331914593 / [deg] Coordinate value at reference point      
CRVAL3  =  7.4106020148532E-07 / [m] Coordinate value at reference point        
LONPOLE =                180.0 / [deg] Native longitude of celestial pole       
LATPOLE =      1.8814331914593 / [deg] Native latitude of celestial pole        
RADESYS = 'ICRS'               / Equatorial coordinate system                   
SIM-TYPE= 'LVF     '                                                            
CHAN    =                    1                                                  
PIXSCALE=                  6.2   ```

@lznakano
Copy link
Contributor Author

@ejoliet
I uploaded one Spherex file that I got in the confluence page. I tested it. As I commented in the ticket, although it seems working, the result is wrong because the header contains several wcsaxes(ka). We need to discuss this issue if we want to handle such headers or not, if we do want to handle it, we can do so. I can issue another ticket to process such headers.

ejoliet
ejoliet approved these changes Jun 17, 2019
View reviewed changes
Copy link
Contributor

@ejoliet ejoliet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at all the tickets with 'cubes' label in it and seems all ok now!
Pretty neat!!!

Only the Spherex case above but it's minor and it shouldn't block the merge of the PR.

I'll mark it as approved, waiting for @robyww to comment. Thanks!!!

robyww
robyww approved these changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@robyww robyww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good. There is some code clean up I would like you to do. After that I think it is ready to go.

src/firefly/js/visualize/FitsHeaderUtil.js Outdated
@@ -129,7 +128,8 @@ export function makeDoubleHeaderParse(header,zeroHeader,altWcs) {
getDoubleAry(keyRoot,altWcs, startIdx,endIdx,def=undefined) {
if (startIdx>endIdx) return def;
const retAry= hp.getDoubleAry(keyRoot,altWcs,startIdx,endIdx);
if (retAry) return retAry;
let arrNaN = retAry.filter( (v)=> isNaN(v) );
if (arrNaN.length!==retAry.length) return retAry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two getDoubleAry functions has different behaviors. the first never passes the default back (unless the parameters are wrong) the other check to see if every elements is null. They really should have the same behavior. If you want failure (therefore default) to be that every element is null then the first should do that as well. If you want this current hybrid behavior you probably should have another function name in makeDoubleHeaderParse

getDoubleAry in makeHeaderParse should to the check to determine if if should return the default or it is not that useful in other context.

src/firefly/js/visualize/projection/WavelengthHeaderParser.js Outdated
}

}
function assignDefault(inArr, keyRoot, which, N){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would be better to name this routine more clearly. It look like a general reusable name to it has a specific purpose.

Also you might want to document it. I am having trouble understanding what it is doing.

src/firefly/js/visualize/projection/WavelengthHeaderParser.js Outdated
*/
function getPC_ijKey(header, zeroHeader,which){

var key;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be let

src/firefly/js/visualize/projection/WavelengthHeaderParser.js Outdated
hasPC = true;
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to make a new routine in FitsHeaderUtil, makeHeaderParse call something like hasKeyStartingWith() and add one to makeDoubleHeaderParse as well.

src/firefly/js/visualize/projection/WavelengthHeaderParser.js Outdated
}
}
if (hasPC && hasCD){
throw Error('The FITs header contains both PC_ij and CD_ij');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to throw exceptions here, That has the potential to break all the new image processing. Just return a a false or something and return an undefined in the calling function.

src/firefly/js/visualize/projection/WavelengthHeaderParser.js Outdated

//We only support the standard format in the FITs header. The standard means the CTYPEka='WAVE-ccc" where
//ccc can be 'F2W', 'V2W', 'LOG', 'TAB' or empty that means linear. If the header does not have this kind
// CTYPEka defined, we check if the it is a spectra cube. If it is a spectra cube, we display the wavelength
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good docs.

src/firefly/js/visualize/projection/WavelengthHeaderParser.js Outdated
@@ -2,12 +2,14 @@
* License information at https://github.com/Caltech-IPAC/firefly/blob/master/License.txt
*/

import {makeDoubleHeaderParse} from '../FitsHeaderUtil.js';
import {makeDoubleHeaderParse,getHeader} from '../FitsHeaderUtil.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove getHeader it is not used

robyww
robyww reviewed Jun 20, 2019
View reviewed changes
src/firefly/js/visualize/projection/WavelengthHeaderParser.js
@@ -7,7 +7,9 @@ import {AWAV, F2W, LINEAR, LOG, PLANE, TAB, V2W, WAVE} from './Wavelength.js';

export function parseWavelengthHeaderInfo(header, altWcs='', zeroHeader, wlTable) {
const parse= makeDoubleHeaderParse(header, zeroHeader, altWcs);
return calculateWavelengthParams(parse,altWcs, wlTable);
const which= altWcs?'1':getWCSAXES(parse);
const mijMatrixKeyRoot = getPC_ijKey(parse,which);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check and return if this value is undefined

@lznakano
FIREFLY-116:
247c9dd 
 1.   Fixed the 0 wavelength at the first plane bug
 2.   Fixed the wrong read out in _data_SOFIA_FIFI-LS_L4_2016-07-05_FI_F316_p3993_g10_F0316_FI_IFS_04015210_BLU_WXY_600284-700065.fits
 3.   Disable the wavelength read out if the header is not the  standard header for wavelength
@lznakano lznakano merged commit b18a737 into dev Jun 20, 2019
@lznakano lznakano deleted the FIREFLY-116 branch June 20, 2019 17:56
@robyww robyww added bug Image FITS images labels Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robyww robyww robyww approved these changes

@ejoliet ejoliet ejoliet approved these changes

Assignees

@azhangipac azhangipac

Labels
bug Image FITS images
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lznakano @ejoliet @robyww @azhangipac