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

pip installable package #54

Merged
merged 35 commits into from Dec 22, 2020
Merged

pip installable package #54

merged 35 commits into from Dec 22, 2020

Conversation

weaverba137
Copy link
Member

This PR:

  • Closes Code cleaning #49.
    • There are some minor aspects of Code cleaning #49 that might not be addressed here, but they can be addressed in separate, small issues and PRs.
  • Refactors the package so it is installable with pip. You can test this right now with pip install git+https://github.com/desihub/prospect.git@pip-installable-package#egg=prospect.
    • There may be a few additional, minor changes needed to be able to export prospect to PyPI.
  • Moves package data files inside the package.
    • While I was doing that, I noticed that the same JavaScript files were being open and read many (many!) times. External .js files are now read only once and their contents are cached.
  • Moves all executable code into prospect.scripts.
    • A lot was there already, this just finishes the process.
  • Adds (bare-bones) unit tests.
    • As a bonus, the unit tests use GitHub Actions instead of Travis.
    • Coverage check is included and working.
  • Adds ReadTheDocs support: https://desi-prospect.readthedocs.io/en/latest/

@weaverba137 weaverba137 self-assigned this Dec 9, 2020
Copy link
Collaborator

@armengau armengau left a comment

Choose a reason for hiding this comment

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

  • Checked changes (however I'm basically not competent to check the pip/sphinx/readthedocs setup...) + basic prospect GUI working in notebook + pip install (but again I'm not experienced with pip).

  • Recommend to remove line 1577 in plotframes.py (line_file_dir var not used anymore)

  • Notebooks need to be adapted (simple change to import prospect.utilities)
    This is an obvious change but users may have their own notebook (in particular to inspect some targetids), which won't work once prospect changes are propagated to desiconda.
    I dont know how to warn them, appart sending an email to VI leads (there are probably very few people doing some VI now).

  • Maybe now is also a good time to change the filename plotframes.py, and maybe the name of the main function in it, ie plotspectra() ? Again this will impact user's notebooks. Maybe something like prospect/create_gui/interface/viewer ... (I dont have any clear idea) ?

  • (For another git branch:) I'd like to restructure the code in plotframes.py: my intend is to make few classes (ProspectCDSData, ProspectPlots, ProspectWidgets), reducing the code length in plotframes.plotspectra(). And also move other functions from plotframes.py to other files. Do you have comments or other ideas?
    Also I could make sure to restructure things so that large fractions of code could be directly used in plotspecutils.

@weaverba137
Copy link
Member Author

@armengau , thank you for the good comments.

  • I'll take a look at line 1577.
  • I'll take a look at the notebooks as well.
    • While I have your attention, could you please address Prospect_demo.ipynb is not actually a notebook #37? It would be great to recover the original notebook if it still exists.
    • The proposed version number, 1.0.0, is a standard signal to expect API changes, so the rename of utils_specviewer.py to utilities.py is within that scope.
    • Thinking further ahead though, we may want to clearly distinguish modules and functions that are intended for end users, versus modules that are primarily for internal use by other functions and modules.
  • If we don't have a clear plan for renaming plotframes.py, I propose we skip that for now. This is already a complex PR.
  • On the other hand, the internal structure of plotframes.py could definitely use a major overhaul. That same overhaul would allow us to merge plotframes.py and plotspecutils.py, since the vast majority of their code is actually the same. However, again, I propose we make that a separate PR.
    • In that particular case, I think we could make the changes while still making minimal changes to the user-visible API.

weaverba137 and others added 4 commits December 11, 2020 10:21
…te. I started editing Prospect_demo (has to be re-written entirely) but cannot finish that before nersc shutdown. Branch can still be merged to master.
@weaverba137
Copy link
Member Author

I've removed the unused variable line_file_dir. I would like to test the notebooks at NERSC after service is restored, and merge after that.

@armengau
Copy link
Collaborator

I've updated and tested Prospect_demo at nersc. It's ok to merge for me.

@weaverba137
Copy link
Member Author

Thank you for the testing @armengau. I'll merge now, and if we need further testing on the notebooks, we can open separate issues for that.

@weaverba137 weaverba137 merged commit 3791b9a into master Dec 22, 2020
@weaverba137 weaverba137 deleted the pip-installable-package branch December 22, 2020 18:29
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

Successfully merging this pull request may close these issues.

Code cleaning
2 participants