foxsi / foxsi-smex

Tools for scientists to investigate the capabilities of FOXSI
MIT License
2 stars 8 forks source link

Adding new spectral routines, update convolution method and changing old routine names. #11

Closed samuel-badman closed 9 years ago

samuel-badman commented 9 years ago

1) Updated names so all files have form foxsiget*.pro 2) Updated convolution method to take any 2d array dimensions, be more accurate and quicker 3) Added 2 new functions: foxsi_get_spectral_image.pro foxsi_get_source_map_spectrum.pro

This extends the capability of foxsi_get_2d_image (previously get_foxsi_image) to process array of images with spectral information and integrate the effective area routines to start to account for the instrument spectral response.

foxsi_get_source_map_spectrum.pro supplies a default source cube to this routine.

ayshih commented 9 years ago

I strongly suggest that you replace your string-based booleans (e.g., inaccurate1) with byte-based booleans (i.e., 0 and 1). It's far better style, and it allows for logical operations.

ehsteve commented 9 years ago

I concur with @ayshih comment.

ehsteve commented 9 years ago

@samuel-badman what is your timeline for addressing these issues?

samuel-badman commented 9 years ago

@ehsteve I'm planning on updating the pull request with changes addressing the comments so far today.

samuel-badman commented 9 years ago

09/08/15 - New commit to pull request. @ehsteve @aringlis @LinErinG @ayshih

I've tried to address all the above comments in this commit: I've neatened out some of the idl functions I'm using as per the suggestions e.g. using the size() where possible and performing the convolution using a nested loop rather than a single variable. I also replaced the string based booleans with 1 / 0 booleans.

I'd like to open a couple more areas for discussion on my changes below

1) Name Changes:

I have tried to make the purpose of each function more obvious. The main functions which actually do the imaging now start with foxsi_get_output... and the functions which produce the default datasets now begin foxsi_get_default_source...

The functions are now: 2d, no energy information: Main: foxsi_get_output_2d_image.pro Default source: foxsi_get_default_source_map.pro

3d, energy dimension included: Main: foxsi_get_output_image_cube.pro Default source: foxsi_get_default_source_cube.pro mk structure: foxsi_make_source_structure.pro (discussed below)

Other: foxsi_get_psf_array.pro (unchanged)

Is this naming structure at all intuitive to the outside eye? Any further suggestions appreciated.

2) User input format

For the case with energy information, the general structure of data that the functions utilise is now an array of maps with appended tags for each energy slice with an upper bin value and lower bin value. The midpoint of the energy bin is used in finding the effective area value for each bin.

The function is now structured such that it takes an input of an array of flux maps (corresponding to different energies) and 2 further pieces of information: the lower bound of the lowest energy bin and the upper bound of the highest energy bin. Then via an external function: foxsi_make_source_structure.pro, this user input is made into the structure format as described above.

If no user input is received, the function uses the default data and an energy range of 1 - 60keV.

In generating the source structure, we currently assume a fixed bin width across the whole spectral range in order to assign an energy value to each bin in between the max and min. What I would like to discuss is whether this is a reasonable constraint? Would it be better to allow the user to fully specify each energy bins lower and upper value separately (e.g via an array of upper and lower values or adding tags individually)? Should we put in the capability in the code for the user to specify what kind of energy information they want to provide?

LinErinG commented 9 years ago

Thanks for these updates. The name changes look fine to me, though the others should also comment.

As for the energy bins, I just tried running the code supplying a map cube and lower/upper energy ranges, and I found that to be quite user-friendly. However, I'd personally also like the ability to choose the energy axis to be non-uniformly sampled. I suggest keeping the min/max energy keywords but adding another keyword that allows the user to input an energy array that specifies all the bin edges (so having dimension n_bins+1). The routine would then need to infer from what's input (min/max keywords or the energy array) to decide what to do.

One more request is to update the imaging.md doc with the new function names so that the code testers can have a clear road map for test-driving the code. Thanks!

samuel-badman commented 9 years ago

Updated the imaging doc and made the variable name changes suggested by Lindsay