Angel #401

angelrls · 2024-03-15T09:25:54Z

Probando... qué desesperación...

git add src/pykoala/plotting/plot_plot.py
git commit -m "Ángel's tool for quick plotting, it works well but needs checking to make arch pretty"
git add src/pykoala/plotting/rss_plot.py
git commit -m "Functions for plotting rss"
git add src/pykoala/ancillary.py
git commit -m "Some extra functions added: vprint, vplot, find_nearest, find_index_nearest"
git add src/pykoala/instruments/koala_ifu.py
git commit -m "Minor changes to print or plot if requested"

…make arch pretty

…earest

src/pykoala/ancillary.py

src/pykoala/instruments/koala_ifu.py

PabloCorcho · 2024-03-15T11:09:52Z

src/pykoala/instruments/koala_ifu.py

-                   header=koala_header,
+                   header=koala_header,                  
                   fibre_table=koala_fibre_table,


All of this information is never stored in the RSS object. Actually, the function read_rss is deprecated and should disappear. Every instrument_rss method within the instruments module should returns an RSS.

If you would like to force the class to include further information (that won't be used anywhere else in the pipeline), you can first create the RSS within koala_rss, and then create new attributes via rss.new_attribute = value.

That is what I actually did, I created my own koalaRSS function that uses koala_rss (this one still calls read_rss) and this function creates a new dictionary rss.koala_info where I add the parameters I want to keep, so far:

koala_info_rss = dict(first_good_wave_per_fibre = None, last_good_wave_per_fibre = None, list_fibres_all_good_values = None, rss_object_name = None, path_to_file = None, n_wave = None, n_spectra = None, spaxel_size = None, aaomega_arm = None, aaomega_grating = None, KOALA_fov = None, pos_angle = None, RA_cen = None, DEC_cen = None, description = None, valid_wave_min = None, valid_wave_max = None, valid_wave_min_index = None, valid_wave_max_index = None )

Somehow, I would have preferred to have at least some of these as rss.item as it is faster, but we agreed we will not add anything else to the rss except an extra rss.info, so that is what I did.

I also created rss.koala_header with all the info of the header and rss.koala_wcs (I'm quite sure I do not need this one, but just in case I'm keeping it for now).

Please post the relevant code, but it doesn't look good. Anything that is specific to KOALA should be within instruments/koala_ifu.py, WEAVE-specific stuff should go into instruments/weave.py, etc.

By the way, shouldn't we call the instrument "koala" rather than "koala_ifu"?

By the way, shouldn't we call the instrument "koala" rather than "koala_ifu"?

Tip
To rename a file in a git repository, you may use the web interface or
git mv <old-name> <new-name>
from the command line. By doing so, you inform git that it is indeed the same file. Otherwise, it will assume that you deleted the old one and created a completely new one, and its whole content will be copied (again) to the repo, wasting disk space.

PabloCorcho · 2024-03-15T11:13:03Z

src/pykoala/instruments/koala_ifu.py

                   fibre_table=koala_fibre_table,
                   info=info
                   )
+
+    # Plot RSS image if requested  #!!!
+    if vplot(**kwargs): rss_image(rss, **kwargs)


This should not be here. The output of rss_image is never returned, and it will only work in interactive environments (e.g, ipython/spyder). We can create a method within RSS that returns this plot and can be called at any time during the reduction/processing.

I would suggest to remove this lines, and implement the rss_image function as a new method of the RSS class.

I agree that rss_image should be common to all RSS objects (i.e. a class method), not just for KOALA.

However, I can imagine, for instance, that a non-interactive pipeline would still want to produce QC plots, and this would be just one of them. The plotting routines should automatically decide whether to show the figure in the screen and/or to save a file. In case of doubt, no screen at all, unless it is specifically requested.

rss_image can save the plot in a file if requested when argument save_plot_in_file is not None, although I have to check how this can be done adding figures in the same file if several figures are requested. I found this extremely useful. If you don't want it there, we can delete it, I call it again in my koalaRSS(). But this is not only for KOALA, it should work for any kind of instrument.

it should work for any kind of instrument
Therefore, it should be somewhere "general", such as rss.py, data_container.py, or plotting/qc_plots.

In any case, wouldn't a name like "plot_spectra" be more informative than plot_plot? Same for x and y: the could would be more readable if they were called something like "wavelength_list" and "spectra_list".

PabloCorcho · 2024-03-15T11:15:52Z

src/pykoala/plotting/plot_plot.py

+# -----------------------------------------------------------------------------
+# -----------------------------------------------------------------------------
+# -----------------------------------------------------------------------------
+def full_path(filename,path, verbose=False):


This function is not necessary as it can be substituted by os.path.abspath(path), that returns the absolute path of an input filename/relative path.

It was in my todo list to do that change, once I knew about os.path.join, but I have not done it yet.

I quickly copy-pasted my version of the file plot_plot.py in the code, but as advised arch is not ideal BUT IT WORKS.

I'm deleting this function and using os.path.join accordingly.

IT WORKS
In a *nix system, probably yes. Definitely not in Windows.

PabloCorcho · 2024-03-15T11:19:14Z

src/pykoala/plotting/plot_plot.py

+               if np.nanmedian(y[0]) < 1E-20 and np.var(y[0]) > 0 : fcal = False
+       for i in range(len(y)):
+           if psym[i] == "":
+               plt.plot(xx[i],y[i], color=color[i], alpha=alpha[i], label=label[i], linewidth=linewidth[i], linestyle=linestyle[i])


Why do you need a for loop, you can pass two lists as x, y arguments to plt.plot, this makes this method extremely inefficient...

I am not sure. I think @angelrls 's idea is that you can tell the routine to plot one spectrum or a list of spectra (with common or individual wavelength). That's why x and y may be single objects or lists. If so, every spectrum (element of y) is expected to have its own color, alpha, etc. Is this correct? I'm not sure what's the best way to handle this, but the loop is probably not too terrible, as any plot would arguably contain a small number of spectra...

x and y can be just a list, plot_plot(wave, intensity), or y can be a list of lists, plot_plot (wave, [intensity[0], intensity[1]]) or two lists of lists, plot_plot ([wave1, wave2], [[intensity[0], intensity[1]]) . Yes, each element can have their own color, alpha, etc

I'm sure there are better ways of coding it, that is why I said that arch could be improved, but it works and in any case it is expected that this will only use few inputs (<10, and typically 2-4 at max).

I particularly find it extremely useful as I don't have to check how to put minor axes, change colours, style, alpha... etc, just check the "help" of plot_plot and go. I had a similar thing for my plots in IDL.

PabloCorcho

I think we should address the comments I have left before merging it into main.

paranoya

Sorry, I started writing this hours ago, but @PabloCorcho has been faster ;^D

In ancillary.py:

We should use log rather than defining vprint
do_nothing = 0 -> pass
if vplot(**kwargs): -> if kwargs.get('plot', False): (if plot is meant to be boolean, there may be more elegant alternatives)
find_index_nearest -> np.searchsorted
find_nearest is somewhat redundant (and never used)

paranoya · 2024-03-15T18:43:59Z

I have a naive question. What does "draft" mean for a Pull Request? Could we edit code and make additional commits (if we had infinite free time ;^D)? Are these three naive questions? ;^D

paranoya · 2024-03-15T18:45:57Z

Another naive question: why could I reply to some comments, but not all of them?

angelrls · 2024-03-15T22:10:06Z

Thanks again. I can't reply Yago's comments, so here they go:

- We should use log rather than defining vprint: - > please, explain who to use log.
- do_nothing = 0 -> pass. Thanks, I've lost a lot of time trying to find something like that...
- if vplot(**kwargs): -> if kwargs.get('plot', False): (if plot is meant to be boolean, there may be more elegant alternatives) -> DONE and answered above
- find_index_nearest -> np.searchsorted -> THANKS, replaced and deleted
- find_nearest is somewhat redundant (and never used) -> I did it for me, but agreed. DELETED

paranoya · 2024-03-16T06:48:46Z

src/pykoala/plotting/plot_plot.py

+              ptitle = None, xlabel = None, ylabel = None, label="",
+              #ptitle="Pretty plot", xlabel="Wavelength [$\mathrm{\AA}$]", ylabel="", 
+              fcal=None, 
+              psym="", color="blue", alpha="", linewidth=1,  linestyle="-", markersize = 10,


color=None, alpha=0.5

paranoya · 2024-03-16T07:12:46Z

src/pykoala/plotting/plot_plot.py

+           xx.append(x)
+   else:
+       xx=x
+


wavelength_list = np.asarray(x) if wavelength_list.ndim == 1: wavelength_list = wavelength_list.reshape((1, wavelength_list.size)) if wavelength_list.ndim != 2: raise ValueError('I need a list of wavelengths!')

and same for spectra_list

Is this what you mean?

paranoya · 2024-03-16T07:15:37Z

src/pykoala/plotting/plot_plot.py

+
+   if xmin is None : xmin = np.nanmin(xx[0])
+   if xmax is None : xmax = np.nanmax(xx[0])  
+


xmin = np.nanmin(wavelent_list)
Why only the first wavelength vector in the list?

paranoya · 2024-03-16T07:18:50Z

src/pykoala/plotting/plot_plot.py

+       if np.isscalar(linewidth): linewidth=[linewidth_]*n_plots
+       if np.isscalar(markersize):markersize=[markersize_]*n_plots
+       if np.isscalar(linestyle): linestyle=[linestyle_]*n_plots
+       if color == "blue" : 


color is None
(otherwise, it is impossible to plot just one blue line)

paranoya · 2024-03-16T07:20:31Z

src/pykoala/plotting/plot_plot.py

+           if ymin == "":
+                    y_min_ = []                
+                    y_min_.extend((y[i][j]) for j in range(len(xx[i])) if (xx[i][j] > xmin and xx[i][j] < xmax) )  
+                    y_min_list.append(np.nanpercentile(y_min_, percentile_min))


Do you mean ymin, ymax = np.nanpercentile(spectra_list, percentile_min, percentile_max)?

paranoya · 2024-03-16T07:32:00Z

src/pykoala/plotting/plot_plot.py

+   plt.tick_params('both', length=tick_size[0], width=tick_size[1], which='major')
+   plt.tick_params('both', length=tick_size[2], width=tick_size[3], which='minor')
+   plt.tick_params(labelsize=axes_fontsize)
+   plt.axhline(y=ymin,linewidth=axes_thickness, color="k")     # These 4 are for making the axes thicker, it works but it is not ideal...


Actually, one should define axes rather than calling plt directly. I guess

ax = plt.gca() for axis in ['top','bottom','left','right']: ax.spines[axis].set_linewidth(axes_thickness) ax.tick_params(width=axes_thickness)

would do the trick.

PabloCorcho · 2024-03-16T07:59:20Z

Draft means that until someone changes the PR status it cannot be merged, it's a way to protect the target branch.
I agree that implementing good verbose and plotting methods is important, but I do believe that the pending Issues should have a higher priority that the cosmetic aspects of the pipeline for now...
@angelrls the suggested changes for this PR can be implemented in your local branch, and once pushed to origin, they will be incorporated in the PR automatically.

… basic mask

angelrls added 4 commits March 15, 2024 16:13

Ángel's tool for quick plotting, it works well but needs checking to …

076234b

…make arch pretty

Functions for plotting rss

56fd9bd

Some extra functions added: vprint, vplot, find_nearest, find_index_n…

89c07f4

…earest

Minor changes to print or plot if requested

2c5d1cf

angelrls requested a review from PabloCorcho March 15, 2024 09:25

PabloCorcho reviewed Mar 15, 2024

View reviewed changes

src/pykoala/ancillary.py Outdated Show resolved Hide resolved

PabloCorcho reviewed Mar 15, 2024

View reviewed changes

src/pykoala/ancillary.py Outdated Show resolved Hide resolved

PabloCorcho reviewed Mar 15, 2024

View reviewed changes

src/pykoala/ancillary.py Outdated Show resolved Hide resolved

PabloCorcho reviewed Mar 15, 2024

View reviewed changes

src/pykoala/instruments/koala_ifu.py Outdated Show resolved Hide resolved

PabloCorcho reviewed Mar 15, 2024

View reviewed changes

PabloCorcho requested changes Mar 15, 2024

View reviewed changes

paranoya reviewed Mar 15, 2024

View reviewed changes

paranoya reviewed Mar 16, 2024

View reviewed changes

angelrls added 5 commits May 9, 2024 16:13

New wavelength correction object

c8230f2

Delete old functions, added read_table function

f8e6c0a

Updated header with good name of rss

a906acf

Updated table for small wavelength shift corrections

476311c

All Angel's functions for procesing KOALA rss

ebbe4ec

angelrls added 4 commits May 9, 2024 16:45

Updated plot_plot

69ac164

Updated functions for plotting rss

444a75d

Added generic task for finding edges with nans and use them to create…

c652975

… basic mask

Added onedspec

f424245

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Angel #401

Angel #401

angelrls commented Mar 15, 2024

PabloCorcho Mar 15, 2024

angelrls Mar 15, 2024

paranoya Mar 16, 2024

paranoya Mar 16, 2024

PabloCorcho Mar 15, 2024

paranoya Mar 15, 2024

angelrls Mar 15, 2024

paranoya Mar 16, 2024

PabloCorcho Mar 15, 2024

angelrls Mar 15, 2024 •

edited

paranoya Mar 16, 2024

PabloCorcho Mar 15, 2024

paranoya Mar 15, 2024

angelrls Mar 15, 2024

PabloCorcho left a comment

paranoya left a comment

paranoya commented Mar 15, 2024

paranoya commented Mar 15, 2024

angelrls commented Mar 15, 2024 •

edited

paranoya Mar 16, 2024

paranoya Mar 16, 2024

paranoya Mar 16, 2024

paranoya Mar 16, 2024

paranoya Mar 16, 2024

paranoya Mar 16, 2024

paranoya Mar 16, 2024

PabloCorcho commented Mar 16, 2024


		if xmin is None : xmin = np.nanmin(xx[0])
		if xmax is None : xmax = np.nanmax(xx[0])

Angel #401

Are you sure you want to change the base?

Angel #401

Conversation

angelrls commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelrls Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PabloCorcho left a comment

Choose a reason for hiding this comment

paranoya left a comment

Choose a reason for hiding this comment

paranoya commented Mar 15, 2024

paranoya commented Mar 15, 2024

angelrls commented Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PabloCorcho commented Mar 16, 2024

angelrls Mar 15, 2024 •

edited

angelrls commented Mar 15, 2024 •

edited