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

Code cleaning #49

Closed
armengau opened this issue Aug 28, 2020 · 4 comments · Fixed by #54
Closed

Code cleaning #49

armengau opened this issue Aug 28, 2020 · 4 comments · Fixed by #54
Labels
question Further information is requested

Comments

@armengau
Copy link
Collaborator

@weaverba137 I know there's probably a lot to do on this side.
I'd like to try to "clean" the code a bit now
A first tentative list (clearly not exhaustive):

  1. Separate DESI dependencies as much as possible (your issue Reduce dependencies on DESI software stack when using prospect with other data sets #47)
  2. main code in plotframe.py: change the names plotframes/plotspectra ? is a main needed ? Move other functions to other files. Reduce code as much as possible in plotspectra() but I don't see much more to do that move js code (most of it was already done)
  3. Code in "myXXX.py": these are basically desispec-code-based functions but doing something slightly differently. I wrote them because at the time of writting them I could not find desispec functions doing exactly what I needed for prospect scripts. They should ideally be in desispec, or removed/renamed... In more details (see their headers):
  • "mycoaddcam": don't coadd over exposures
  • "myspecselect": includes selection based on EXPID and indices
  • "myspecupdate": always append new spectra; don't rely necessarily on EXPID
  1. move/rename "utility" python functions?

I'd like to have your feedback and suggestions... thanks!

@weaverba137
Copy link
Member

@armengau, these are all good suggestions. To these I would add:

  1. Move Python code that is currently in the script files into e.g. prospect.scripts and have minimal executables that call the functions in the package. desispec has examples of this.
  2. Move the js and template directories into the package itself, so that the files can be found with e.g. pkg_resources.resource_filename().
  3. Start setting up the doc/ directory with Sphinx configuration files.

That makes a big list. Each one could be a separate issue.

@armengau
Copy link
Collaborator Author

armengau commented Jan 8, 2021

@weaverba137 I'd like now to restructure essentially the plotframes.py code in a new branch. On the other hand plotspecutils is largely redundant with plotframes. Does it make sense to fully merge both or do you want to keep them separate?
What do you think of the following:

  1. Maybe first merge plotspecutils with plotframes? Looking at their diff, it looks easy. Maybe also find a different name: in the end, external users or scripts use eg "from prospect import MODULE" then "MODULE.plotspectra()".
    Well, for this first step I'm not sure what to do.

  2. Create few classes, one file/class, containing the main data in plotframes.plotspectra(). Once this is done, plotframes.plotspectra() should have a reasonable nb of code lines. Suggested classes, with the associated most important data:

  • class ProspectCDS:
    cds_spectra
    cds_model
    cds_targetinfo
  • class ProspectPlotSet:
    fig
    data_lines
    noise_lines
    model_lines
    zoomfig
    imfig
    emission/absorption lines (should be renamed)
  • class ProspectWidgetSet:
    ifiberslider (could we rename it to ispectrumslider ?)
    smootherslider
    prev/next_button
    several z widgets
    target info display
    several VI widgets (may be an independent class)
  1. Move remaining functions in plotframes.py to separate files (eg one file for emission/abs line functions).
    I don't know what to do with the main. I would remove it entirely (there's no main in plotspecutils).

  2. Remove unused functions in utilities.py

@armengau armengau added the question Further information is requested label Jan 8, 2021
@weaverba137
Copy link
Member

I'm very confused as to why you needed to reopen an existing issue? The original issue had to do with restructuring the package into a standard Python, pip-installable format. Restructuring plotframes.py is completely independent of that, and really should be a separate issue.

@armengau
Copy link
Collaborator Author

armengau commented Jan 8, 2021

Alright, I did it because I thought it was relevant to use the same issue (a related topic). I guess reopening issues in github is not a good practice.

@armengau armengau closed this as completed Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants